Closed Bug 1675670 Opened 4 years ago Closed 4 years ago

Remove mozilla::Result from ParserAtoms code

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

RESOLVED FIXED
85 Branch
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.

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.

(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.

I've written up some info on the problem as Bug 1675783 where we can address the general problem.

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.

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7da5f1f192f6 Remove mozilla::Result from ParserAtoms. r=jandem
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: