Bug 278488
| Summary: | [WTF] Transition non JSC and WebCore code to use a checked HashMap (3/3) | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | zak ridouh <zakr> |
| Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | cdumez, ggaren, rniwa, webkit-bug-importer, zakr |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
zak ridouh
We should have WTF HashMap be safe by running `checkKey()`, by default on the HashMap API, as well as on the underlying HashTable container. This bug is for tracking step 1 of this transition.
Transition plan/process:
- Add a template parameter (ShouldValidateKey::Yes/No) for `HashTable` and `HashMap`, with default being Yes in release builds as well.
- Elevate ASSERTs to RELEASE asserts based on this conditional for release builds.
- Activate key validation in WebKit (excluding WebCore and JSC for this step)
- Monitor performance impact
- Transition HashSet and other Hash* types to this structure
- Turn off key validation on hot paths on a case by case basis, to preserve performance in the engine.
Open to suggestions.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Radar WebKit Bug Importer
<rdar://problem/134445131>
zak ridouh
Correction about a typo on last point in the transition plan:
- Turn ON key validation slowly in WebCore/JSC on a case by case basis, to preserve performance in the engine.
Geoffrey Garen
Note: We really only need to RELEASE_ASSERT checkKey.
Side point: This seems suspicious:
if (!HashFunctions::safeToCompareToEmptyOrDeleted)
return;
It appears that the assertion skips the validation for some types.
Geoffrey Garen
I think you could probably just change checkKey to test isHashTraitsEmptyValue<KeyTraits> and KeyTraits::isDeletedValue, right?
zak ridouh
Pull request: https://github.com/WebKit/WebKit/pull/32667
zak ridouh
This bug is tracking the 3rd step of the checkedKey HashMap transition plan. We currently now have `UncheckedKeyHashMap` for performance sensitive code, and `HashMap` which does check keys for all other code.
This is the current step:
- Activate key validation in WebKit (excluding WebCore and JSC for this step)
- Monitor performance impact
zak ridouh
Pull request: https://github.com/WebKit/WebKit/pull/35511
EWS
Committed 285616@main (bbf5bdae3c29): <https://commits.webkit.org/285616@main>
Reviewed commits have been landed. Closing PR #35511 and removing active labels.