Closed Bug 1342439 Opened 3 years ago Closed 3 years ago

Clean up and optimize overrecursion checks

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

The overrecursion checks call js::RunningWithTrustedPrincipals to decide which stack limit to use (trusted vs untrusted). This call to RunningWithTrustedPrincipals is not inlined and it often shows up in profiles.

I'll attach patches to (1) convert these gnarly macros to MOZ_ALWAYS_INLINE functions to make them a lot more maintainable (2) change the overrecursion check to try the untrusted stack limit first, so we avoid the slow call to RunningWithTrustedPrincipals when that succeeds (99.9% of the time).
Large patch but very mechanical.
Attachment #8840921 - Flags: review?(luke)
As described in comment 0.
Attachment #8840922 - Flags: review?(luke)
Comment on attachment 8840921 [details] [diff] [review]
Part 1 - Replace macros with functions

Review of attachment 8840921 [details] [diff] [review]:
-----------------------------------------------------------------

But by abandoning our ancient macros, do we not also abandon _ourselves_?
Attachment #8840921 - Flags: review?(luke) → review+
Comment on attachment 8840922 [details] [diff] [review]
Part 2 - Optimize CheckRecursionLimit

Review of attachment 8840922 [details] [diff] [review]:
-----------------------------------------------------------------

I suppose the alternative would be to have some JSAPI-visible base class of JSCompartment that exposed principals, but that's always a messy route so this makes sense.
Attachment #8840922 - Flags: review?(luke) → review+
Can't we remove js::RunningWithTrustedPrincipals and use JSContext::runningWithTrustedPrincipals() directly?
(In reply to Tom Schuster [:evilpie] from comment #5)
> Can't we remove js::RunningWithTrustedPrincipals and use
> JSContext::runningWithTrustedPrincipals() directly?

These functions are also used outside SpiderMonkey, and JSCompartment is in an internal header file... We could give them different functions, maybe uninlined APIs because none of the Gecko callers look very hot, but that requires duplicating some of them so I thought this approach would be simpler.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fbd0ba0c3d7
part 1 - Replace macros to check for overrecursion with functions. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/db093c5d86e6
part 2 - Optimize CheckRecursionLimit to avoid uninlined RunningWithTrustedPrincipals call. r=luke
https://hg.mozilla.org/mozilla-central/rev/4fbd0ba0c3d7
https://hg.mozilla.org/mozilla-central/rev/db093c5d86e6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Jan, I saw RunningWithTrustedPrincipals coming up when profiling bug 1342713, and I filed bug 1342721 as a result.  The patch there eliminates that cost from my profile but unfortunately I have a stack of tens of patches and your patch totally conflicts with mine, so I can't test it.  Do you mind please testing your patch on the google spreadsheet scrolling test case and see if there's anything else to be done, and if not, close bug 1342721?  Thanks!
Flags: needinfo?(jdemooij)
Duplicate of this bug: 1342721
(In reply to :Ehsan Akhgari from comment #9)
> Jan, I saw RunningWithTrustedPrincipals coming up when profiling bug
> 1342713, and I filed bug 1342721 as a result.  The patch there eliminates
> that cost from my profile but unfortunately I have a stack of tens of
> patches and your patch totally conflicts with mine, so I can't test it.  Do
> you mind please testing your patch on the google spreadsheet scrolling test
> case and see if there's anything else to be done, and if not, close bug
> 1342721?  Thanks!

Closed it as duplicate. Sorry about that! We had this perf issue for years and then within a few days both of us fix it...
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.