1018568 - Eliminate AutoAssertOnGC overhead in opt builds

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 1

[Jan de Mooij [:jandem]]

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?

Comment 2

[Jan de Mooij [:jandem]]

Oh nevermind, AutoAssertOnGC doesn't touch inUnsafeRegion if we're not on the main thread.

Comment 3

[Jan de Mooij [:jandem]]

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?

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Alternative patch

What I described in comment 4.

Comment 6

[Terrence Cole [: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.

Comment 7

[Jan de Mooij [:jandem]]

OK, pushed the first patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd75df9c682

Comment 8

[Hannes Verschore [:h4writer]]

Confirmed fixed on x86 mac and windows builds. Thanks!

Comment 9

[Hannes Verschore [:h4writer]]

(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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bd75df9c682