Optimize Key::EncodeAsString
Categories
(Core :: Storage: IndexedDB, task)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox80 | --- | fixed |
People
(Reporter: sg, Assigned: sg)
Details
Attachments
(2 files)
Running the http://nparashuram.com/IndexedDB/perf/index.html#Comparing%20Keys shows Key::EncodeAsString as a hotspot. It can be optimized quite easily by doing the iteration over the raw pointer ranges rather than the Span iterators, which contains redundant release-mode assertions for bounds checks that cannot fail here, but the compiler seems unable to optimize them away.
|
Assignee | |
Comment 1•5 years ago
|
||
|
|
Assignee | |
Comment 2•5 years ago
|
||
Depends on D82354
|
||
Comment 3•5 years ago
|
||
Do you think there are other places which could be affected ?
Can something be done about the redundant assertions ?
|
|
Assignee | |
Comment 4•5 years ago
|
||
(In reply to Jan Varga [:janv] from comment #3)
Do you think there are other places which could be affected ?
Definitely, but I think we should only do something about them if they were identified as a hot spot by profiling. We "just" have to do that more systematically.
Can something be done about the redundant assertions ?
Well, for the particular case of Span, there have been some reductions (completely removal vs. downgrading them from release-mode assertions) of its assertions in the recent past (see, e.g. Bug 1635359 and Bug 1649704). This might be easier to optimize away for the compiler than the nsTArray's iterators' assertions, e.g., since it could detect easier that the base pointer and the length do not change, but apparently that doesn't always happen. If we had a range that could only be used in a range-based for, we could use iterators that do not perform bounds checks, as it is syntactically guaranteed that the bounds cannot be exceeded. If it is used in some other way where the iterators are exposed to user code, they could make a mistake and advance an iterator past its bounds, so the bounds checks make sense in that more general case. However, I don't know of a way to ensure a range is only used in a range-based for. I will do some research on that, though.
|
|
Assignee | |
Comment 5•5 years ago
|
||
(In reply to Simon Giesecke [:sg] [he/him] from comment #4)
(In reply to Jan Varga [:janv] from comment #3)
However, I don't know of a way to ensure a range is only used in a range-based for.
Well, maybe we can, mostly. E.g. the ranges of nsTObserverArray use an end-sentinel rather that a normal end iterator of the same as the begin iterator. I think as a consequence of this, you can't use them syntatically with STL algorithms, which IIUC require the same type of begin and end iterators. You could still manually advance the begin iterator past the end, but maybe that's too unlikely to be a real problem. Obviously, we could implement some static analysis to enforce the use of certain range types (with non-bounds-checked iterators) outside of a range-based for.
|
|
||
Comment 6•5 years ago
|
||
Yeah, that all sounds good. Thanks for the information. I'm going to review attached patches.
|
||
Comment 8•5 years ago
|
||
Backed out 2 changesets for causing failures in EncodeAsString.
Backout link: https://hg.mozilla.org/integration/autoland/rev/d6a303ca6f56771b133a768b5ffc93c301a823cd
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308820607&repo=autoland&lineNumber=2185
|
|
Assignee | |
Comment 10•5 years ago
|
||
I fixed the issue and relanded it now.
|
||
Comment 11•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/637bf1e1ee6b
https://hg.mozilla.org/mozilla-central/rev/1da33c3c35c3
Description
•