Open Bug 1338802 Opened 3 years ago Updated 8 months ago

CrossCompartmentWrapper handling is slow on Google Spreadsheet

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

Tracking Status
platform-rel --- +

People

(Reporter: smaug, Unassigned)

References

(Blocks 6 open bugs)

Details

(Keywords: perf, Whiteboard: [qf:p2][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets])

+++ 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.
Could someone more familiar with JS engine take a look?
Blocks: cpg
Flags: needinfo?(nihsanullah)
Just the hashtable lookups from cross compartment wrapping code take >5% of the total time.
Depends on: 1338834
platform-rel: --- → +
Depends on: 1338880
Depends on: 1338894
Depends on: 1339411
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.
Page is just using iframes? The principals are the same, since bug 1339251 gets rid of pretty much all the principal checks.
Depends on: 1339507
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....
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...
(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.
(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.
Blocks: 1342713
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.  :(
(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.
Whiteboard: [platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets] → [qf:investigate][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets]
Flags: needinfo?(nihsanullah)
Whiteboard: [qf:investigate][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets] → [qf:p1][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets]
Assignee: nobody → jorendorff
Flags: needinfo?(jorendorff)
Discussion is ongoing about how to reduce this overhead.
Flags: needinfo?(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.
(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.
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.
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.
CrossCompartmentWrapper stuff also shows up in Speedometer V2 profiles, but it's unclear how much of it is due to CCW specific overhead...
Did we figure out what (if anything) to do here?
Flags: needinfo?(jorendorff)
See comment 14. I guess we could close this bug.
Isn't jorendorff working on this?
Not this one but CCW overhead in general, I think. This bug is/was about CCW on Google Spreadsheets.
jorendorff's work is being tracked in bug 1357862, FWIW.
Whiteboard: [qf:p1][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets] → [qf:p2][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets]
Keywords: perf
Whiteboard: [qf:p2][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets] → [qf:p3][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets]
Flags: needinfo?(jorendorff)
Whiteboard: [qf:p3][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets] → [qf:p2][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets]
Assignee: jorendorff → nobody
Priority: -- → P2
Version: 36 Branch → unspecified
Blocks: 1353284
You need to log in before you can comment on or make changes to this bug.