1338802 - CrossCompartmentWrapper handling is slow on Google Spreadsheet

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

+++ This bug was initially created as a clone of Bug #1237058 +++

I was profiling scrolling (using touchpad's 2-finger scrolling when cursor is over the 'comment' column) of https://docs.google.com/spreadsheets/d/10UeyRoiWV2HjkWwAU51HXyXAV7YLi4BjDm55mr5Xv6c/edit?pref=2&pli=1#gid=368835050
and plenty of the time is spent under CrossCompartmentWrapper::call.
Seems like lots of new Proxy objects are being created (creation taking ~7% of total time!), which also leads to rather frequent minorGCs.
(profile looks pretty much the same in e10s-child-process and non-e10s)




Bug 1237058 fixed some of the GC part of this, but cross compartment handling is still very slow.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Could someone more familiar with JS engine take a look?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Just the hashtable lookups from cross compartment wrapping code take >5% of the total time.

Comment 3

[Steve Fink [:sfink] [:s:]]

Do we know why there are multiple compartments involved here in the first place? It would be nice to know the principals of the compartments involved.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Page is just using iframes? The principals are the same, since bug 1339251 gets rid of pretty much all the principal checks.

Comment 5

[Boris Zbarsky [:bzbarsky]]

If the principals are pointer-identical (as comment 4 seems to suggest), then this is just because the page is creating about:blank iframes or whatnot....

Comment 6

[Boris Zbarsky [:bzbarsky]]

I should note that commoning up compartments whose principals are pointer-identical into a single compartment is _much_ simpler than the general "no wrapper for same-origin" thing, because having a pointer-identical principal is an invariant, while "same-origin" isn't quite.  Still a bit of a pain, of course...

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #6)
> I should note that commoning up compartments whose principals are
> pointer-identical into a single compartment is _much_ simpler

But still very, very complex.

Comment 8

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #6)
> I should note that commoning up compartments whose principals are
> pointer-identical into a single compartment is _much_ simpler than the
> general "no wrapper for same-origin" thing, because having a
> pointer-identical principal is an invariant, while "same-origin" isn't
> quite.  Still a bit of a pain, of course...

FWIW bug 1340710 is making principal comparisons for same-origin principals as cheap as a few integer comparisons.

Comment 9

[Boris Zbarsky [:bzbarsky]]

It's not a matter of how expensive principal comparisons are.  It's a matter of whether the compartment boundary needs to be able to change dynamically or not.

And even then, as bholley pointed out "pointer-identical principals" is not currently a compartment invariant.  :(

Comment 10

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> It's not a matter of how expensive principal comparisons are.  It's a matter
> of whether the compartment boundary needs to be able to change dynamically
> or not.

Yeah, I found that out after profiling with my patches.  :(

> And even then, as bholley pointed out "pointer-identical principals" is not
> currently a compartment invariant.  :(

FWIW I didn't mean pointer-identical principals.

--------

It seems that with our current setup, anything remotely close to fast scrolling is completely out of reach.  I wonder if we should try things like using other data structures for crossCompartmentWrappers, because it seems like the current hash table lookup is too slow without hitting too many collisions...  We really need some JS engine help here.

Comment 11

[Jason Orendorff [:jorendorff]]

Discussion is ongoing about how to reduce this overhead.

Comment 12

[Jason Orendorff [:jorendorff]]

Filed bug 1357862. But I still want more evidence that this is the right thing to be working on.

If you have profiles that show time spent in cross-compartment wrapper code, please share them. I haven't seen a profile of Google Sheets that shows this; they must have changed their implementation in the past 2 months, to use fewer iframes or something.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> If you have profiles that show time spent in cross-compartment wrapper code,
> please share them. I haven't seen a profile of Google Sheets that shows
> this; they must have changed their implementation in the past 2 months, to
> use fewer iframes or something.

Have you measured how many wrappers we allocate when we load the sheet in comment 0 + scroll down for a few seconds? I measured this back in February and we allocated like 800,000 wrappers when doing that. Also we had like 40,000 CCW calls (not allocations) when loading Twitter and Facebook.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Something has changed in Spreadsheets and we have also changed code, like optimizing some CCW and 
merging some wheel events etc. so it doesn't seem to behave as badly.

But compartment-per-global bug has still regressions bugs, for example bug 774119.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

FWIW, the very latest Nightly (55.0a1 (2017-05-13)) has some more wheel handling changes and may in practice make this particular bug less useful.

But that doesn't mean that cross compartment wrapper slowness wouldn't be a bad issue, bug 774119 as an example.

But perhaps we could close this one and make the bugs where CCW work happens qf:p1.

Comment 16

[(no longer active)]

CrossCompartmentWrapper stuff also shows up in Speedometer V2 profiles, but it's unclear how much of it is due to CCW specific overhead...

Comment 17

[Andrew Overholt [:overholt]]

Did we figure out what (if anything) to do here?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

See comment 14. I guess we could close this bug.

Comment 19

[Bobby Holley (:bholley)]

Isn't jorendorff working on this?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Not this one but CCW overhead in general, I think. This bug is/was about CCW on Google Spreadsheets.

Comment 21

[(no longer active)]

jorendorff's work is being tracked in bug 1357862, FWIW.

Comment 22

[Jan de Mooij [:jandem]]

Google fixed this at some point (see comment 14). We've also moved to same-compartment realms for same-origin realms, which would likely have helped here too.