Remove mozilla::Result from ParserAtoms code
Categories
(Core :: JavaScript Engine, task, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox85 | --- | fixed |
People
(Reporter: tcampbell, Assigned: tcampbell)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
There are costs that come from using Result types and we should be careful if the case boils down to non-null-pointer OR oom. For the ParserAtoms code, the use of Result hasn't been needed to detect bugs so we should remove it to speed things up slightly.
On large scripts I see roughly 0.5% of the syntax parse (no BCE or StencilInstantiate) time is saved by removing. This isn't enormous, but since we haven't gained (beyond the early development cycle), we should remove and follow our normal nullptr-for-pending-exception convention.
|
Assignee | |
Comment 1•4 years ago
|
||
Now that the ParserAtom code has matured a bit, we haven't been having issues
with exception reporting and the use of Result isn't providing much value.
This patch removes that plumbing to instead use nullptr for errors which is
more ABI friendly and the more common pattern in SpiderMonkey code.
|
||
Comment 2•4 years ago
|
||
(In reply to Ted Campbell [:tcampbell] from comment #0)
There are costs that come from using Result types […]
If this is the case this is not the intent, file a bug to get this fixed as part of the Result handling.
As this is used all over Gecko, fixing the Result specialization for this use case would have a bigger impact all across Gecko, than just removing its uses here, which has a potential security aspect.
|
|
Assignee | |
Comment 3•4 years ago
|
||
I've written up some info on the problem as Bug 1675783 where we can address the general problem.
|
|
Assignee | |
Comment 4•4 years ago
|
||
I should also add, that our uses of Result are quite different than the typical case in Gecko (which using E=nsresult), so I don't believe we'd see wins in Gecko if we fixed our cases.
|
||
Comment 6•4 years ago
|
||
| bugherder | ||
Description
•