Eliminate AutoAssertOnGC overhead in opt builds

RESOLVED FIXED in mozilla32

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
Created attachment 8432078 [details] [diff] [review]
Patch

I just landed some string patches and they regressed Sunspider, in particular base64. I tracked it down to an AutoCheckCannotGC I added, it's probably the TLS lookup in the AutoAssertOnGC constructor.

This patch makes AutoAssertOnGC a nop in opt builds and this fixes the regression locally.
Attachment #8432078 - Flags: review?(terrence)
(Assignee)

Comment 1

4 years ago
I should probably also use the constructor that takes a JSRuntime*, I tested and it's faster.

Terrence, I think there's also a problem with string methods that are called (or could be called) from other threads. Should we move inUnsafeRegion to PerThreadData?
Flags: needinfo?(terrence)
(Assignee)

Comment 2

4 years ago
Oh nevermind, AutoAssertOnGC doesn't touch inUnsafeRegion if we're not on the main thread.
Flags: needinfo?(terrence)
(Assignee)

Comment 3

4 years ago
There is a MOZ_CRASH in AutoAssertOnGC::VerifyIsSafeToGC, if we want to keep that in opt builds maybe we could have two different classes, one debug-only and one also enabled in opt builds?
(Assignee)

Comment 4

4 years ago
We could also change AutoCheckCannotGC to only inherit from AutoAssertOnGC in debug builds.

This makes sense I think because AutoCheckCannotGC is checked by the static analysis, so the AutoAssertOnGC there is nice to have in debug builds but it's not very important for opt builds.

Let me know what you prefer.
(Assignee)

Comment 5

4 years ago
Created attachment 8432084 [details] [diff] [review]
Alternative patch

What I described in comment 4.
Attachment #8432084 - Flags: feedback?(terrence)
Comment on attachment 8432078 [details] [diff] [review]
Patch

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

Sorry for the bustage! I somehow forgot that we are now using this in every single string user now.

Given we haven't seen a crash spike from this in the last day or two, I think restoring the previous behavior is the right choice.
Attachment #8432078 - Flags: review?(terrence) → review+
Attachment #8432084 - Flags: feedback?(terrence)
Confirmed fixed on x86 mac and windows builds. Thanks!
(In reply to Hannes Verschore [:h4writer] from comment #8)
> Confirmed fixed on x86 mac and windows builds. Thanks!

*fixed regression of ss-base64 and octane-PdfJS
https://hg.mozilla.org/mozilla-central/rev/3bd75df9c682
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.