937287 - GetIonContext() is slow with NSPR

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

GetIonContext() is used to get the current IonContext and is very hot during JIT compilation. All it does is call PR_GetThreadPrivate.

I benchmarked calling GetIonContext() a million times on OS X, with NSPR this takes 12-13 ms. A profile shows a lot of time in PR_GetCurrentThread and PR_GetThreadPrivate.

With the NSPR emulation Bill added, it takes 4 ms. After moving PR_GetThreadPrivate to PosixNSPR.h it's 2-3 ms.

I think it's worth using the PosixNSPR version somehow. We should also investigate getting rid of some GetIonContext() calls, but many are from places like TempObject's "operator new" so it's probably not easy.

Comment 1

[Luke Wagner [:luke]]

I'm in favor of GetIonContext()-removal.  Any estimate on whether we'd see any benchmark speedup (by counting GetIonContext() calls)?  For 'new', we can write:

  new(alloc()) MBasicBlock(...);

It'd be a mondo patch, but, once we have agreement on the patterned changes it could be rubber-stamped.

Also, GetIonContext() (viz., the 'cx' and 'runtime' members) is a simple way to totally circumvent the type-based protection bhackett has been maintaining with ExclusiveContext et al.  We've mentioned this before as something we'd like to fix (at least the removal of those two fields, but the whole struct would be even better).

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #1)
> I'm in favor of GetIonContext()-removal.  Any estimate on whether we'd see
> any benchmark speedup (by counting GetIonContext() calls)?

I counted the number of calls to CurrentIonContext in Octane. There are about 22 million of them, Octane-Typecript accounts for > 7.3 million of them (it compiles a lot of code...). I don't have numbers but it should help a few % on this test at least.

> For 'new', we can write:
> 
>   new(alloc()) MBasicBlock(...);

I just measured and > 5 million of the 7.3 million calls in Octane-Typescript are from IonAllocPolicy and TempObject's operator new. So this seems like a nice way to eliminate most of them...

Comment 3

[Till Schneidereit [:till]]

GetIonContext is gone