95840 - global property access should be made faster

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Johnny Stenback (:jst)]

Currently access to global properties is slowed down by the security check, even
by the optimization that's done to not do the security check in the case where
the object we're accessing properties on is the global object in the context.
This code can be optimized further, I'll attach a patch that makes global
property access ~4 times faster in debug builds, the test used is simply to time
how long it takes to execute while (++i < ahugenumber).

Testcase and patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Testcase

Comment 2

[Johnny Stenback (:jst)]

Attachment: Proposed fix

Comment 3

[Johnny Stenback (:jst)]

Mitchell, could you have a close look at the attached patch? Should we have
ckritzer beat on a build with these changes before checking in?

Comment 4

[vidur (gone)]

sr=vidur. There's a small issue with the possibility that a wrapper will be
reused (making the caching invalid), but generally a context and a global
wrapper have similar lifetime.

Comment 5

[Johnny Stenback (:jst)]

This patch gives us a ~45% speedup with the attached testcase in optimized builds.

Comment 6

[R.K.Aa.]

tried to use this patch, but it failed

Comment 7

[Johnny Stenback (:jst)]

In what way?

Comment 8

[R.K.Aa.]

patch: **** malformed patch at line 110: JSVAL_TO_STRING(id) == sLocation_id;

Comment 9

[Johnny Stenback (:jst)]

Attachment: Updated patch, merged to the tip and contains Vidur's suggestions.

Comment 10

[Johnny Stenback (:jst)]

R.K.Aa, could you try the latest patch, the old ones had rotted...

Comment 11

[R.K.Aa.]

patches, builds, runs. All is well.

Comment 12

[R.K.Aa.]

Building w/RH7.1 errata gcc 2.96-85. Testcase got around 3 times faster after
patching. Building with disable-debug, disable-tests, disable-logging,
enable-crypto, enable-optimize=-O2

Comment 13

[Mitchell Stoltz (not reading bugmail)]

r=mstoltz. Looks safe to me.

Comment 14

[John Bandhauer]

brendan aksed in email...

> If jband addresses vidur's sr= comment concern
>
> There's a small issue with the possibility that a wrapper will be
> reused (making the caching invalid), but generally a context and a global
> wrapper have similar lifetime.
> I would not object to this going in now.  How is a wrapper reused for 
> the global window?  Does the invalidation just cause a performance hit, 
> not any correctness problem?
>
> /be

I think jst got it right in the second patch (good that vidur pointed out the
issue!)

The normal mode for xpconnect wrapped natives is to build a new wrapper for each
scope - global object - to which the native is getting reflecting. For the DOM
support we added a PreCreate hook that the native helper can use to force the
parentage of the wrapper's JSObject. The hook is presented with a proposed
parent for the new wrapper JSObject and can reply with a differrent parent that
xpconnect will then use. This allows DOM JSObject parent chains to come out
right to satisfy the DOM fu. This same mechaism is used in the window case to
force xpconnect to always use only one wrapper for a window even when reflected
into other scopes. When xpconnect receives a different parent back from the
hook, xpconnect looks to that parent's global and if this does not match the
global it started with then xpconnect restarts the wrapping process using that
global as the scope. This will make it *find* the existing wrapper for that
'new' global object and not create a new wrapper as it had begun to do. This
yields the one and only one wrapper for the window. It only needs to do this
dance when reflecting the window into the 'view' of objects in other windows.

That window wrapper lives as long as any native holds a ref to it - which the
DOM does for the life of the window. The pointer could be reused after that - if
malloc coughs it up again. So the invalidation code is certainly the right thing
to do. I'm not seeing any correctness problem with the invalidation as written.

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 47636 [details] [diff] [review]
Updated patch, merged to the tip and contains Vidur's suggestions.

a=asa on behalf of drivers.

Comment 16

[Johnny Stenback (:jst)]

Asa added the above a= by mistake, this will land on the trunk once it opens and
then I'll check it in on the branch.

Comment 17

[Johnny Stenback (:jst)]

Fixed on the trunk.

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Pretty significant performance increase. Additionally the API changes in this
bug help Mitch to land some security fixes. Therefore nsbranch+.

Comment 19

[Johnny Stenback (:jst)]

Attachment: Same as above, but with a typo fixed.

Comment 20

[chris hofmann]

are the api changes exposed to just mitch, or might they be exposed to other 
embedders that are trying to get stable on the 0.9.4 branch?

Comment 21

[Judson Valeski]

AFAICT, these api mods are to private core APIs.

if this is a big core perf improvement, 0.9.4 would be nice.

Comment 22

[Jaime Rodriguez, Jr.]

PDT+. Pls check it into today.

Comment 23

[Johnny Stenback (:jst)]

Fix checked in on the m094 branch.

Comment 24

[Prashant Desale]

verified on 2001-10-04-05-0.9.4. need to verify on trunk.