Closed Bug 1018568 Opened 6 years ago Closed 6 years ago

Eliminate AutoAssertOnGC overhead in opt builds

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
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)
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)
Oh nevermind, AutoAssertOnGC doesn't touch inUnsafeRegion if we're not on the main thread.
Flags: needinfo?(terrence)
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?
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.
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.