1125423 - consider removing Class::{thisObject,outerObject,innerObject} hooks, storing JSCompartment::outerWindow instead

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

I think we can use CPG invariants to remove these confusing hooks:

The thisObject hook must currently be called when using a scope object as 'this'.  This currently serves two very specific purposes: outerizing content globals and replacing With scope objects with the with's object.  Instead of having an open-ended hook, it seems like we could, instead:
 - for With, just do the mapping directly in GetThisObject if is<WithObject>()
 - for content globals, iiuc, class->thisObject just forwards to this->outerObject.  I believe outerObject must always produce the same JSObject* for a given JSObject* input, so I was thinking we could just store this outerized window as JSCompartent::outerWindow.

Going a bit further, we probably don't need a fully general outerWindow hook for the one case of content globals.  Instead it seems like we could just make JSCompartment::outerWindow initially default to JSCompartment::global, use cx->outerWindow whenever we'd currently outerize cx->global, and then provide a JSAPI for setting outerWindow to the outer window when appropriate.

Lastly, given all this, it seems like the innerObject hook could be replaced with the expression:
  (obj == obj->compartment->outerWindow ? obj->compartment->global : obj)

A nice benefit is that these hooks aren't the fastest so we might get a speed boost if they show up on any hot paths.

I'm not sure all these assumptions are valid, though (like there being a single inner-/outer-izable object in any given compartment; one might hope...), cc'ing people who could double check me on this.

Comment 1

[Bobby Holley (:bholley)]

Sounds good to me.

The assumption that the outer window has a consistent identity comes from the fact that the outer is either a bonafide nsOuterWindow or a cross-compartment wrapper to one, and that the mechanics of swap guts mean that we end up always reusing the same JSObject* no matter how many times we transition between the two. The one wrinkle is nuking, where we explicitly remove a wrapper from the wrapper map and swap out proxy handler for DeadObjectProxyHandler. So we need to think about what happens in the nuking case. Naively it seems like it might be ok - when the window is non-current, attempts to re-wrap the global should just land us back with the nuked outer stored on the compartment. And if we ever navigate back to being current, the nuked outer should transform back into an nsOuterWindow. We should definitely add tests for this case though.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> it seems like the innerObject hook could be replaced

Can it?  What happens when we try to innerize a cross-compartment proxy to an outer window right now?

Apart from that, the above plan sounds lovely.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> > it seems like the innerObject hook could be replaced
> 
> Can it?  What happens when we try to innerize a cross-compartment proxy to
> an outer window right now?

Nothing. The outer window proxy has an innerize hook, but the CCW pointing to it does not.

Comment 4

[Luke Wagner [:luke]]

Thinking more about this CCW-to-outer case, it seems like there are two cases:
 1. innerization for the scope chain
 2. innerization for ad hoc stuff we do inside JS (I see JIT stuff, watch stuff, debugger stuff)
For 2, it seems like nbd; we'll treat the CCW like any other non-global object.  For 1, we currently have a hard assumption that the root of the scope chain is a proper GlobalObject which a CCOW-outer-window is not.  Thus, it must already be the case that we never call Execute/Evaluate with a CCW-outer-window.  Is that right?

(Also, bz, do we currently have the invariant that the only non-builtin-scope-object on the scope chain is a global object?  I keep forgetting the current vs. planned state of this.  If so, are we now able to lock down our JSAPIs to stop taking 'obj' args and instead use cx->global?)

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Thus, it must already be the case that we never call Execute/Evaluate with a
> CCW-outer-window.

That sounds plausible, yes.

> do we currently have the invariant that the only non-builtin-scope-object on the scope
> chain is a global object

Not yet.  This is bug 1097987 but I haven't had a chance to work on it...  If someone wants to pick it up, that would be great. We have versions of those APIs that pass an explicit scope chain and then use With objects, but we need to convert all consumers to those...  I haven't had a chance to figure out whether this would work as-is for all consumers yet.

Comment 6

[Luke Wagner [:luke]]

Ah, thanks for reminding me; it's silly how quickly I forget the current state of all that.  But, on the bright side, I get to re-experience anew how excited I am about that bug and the broader scheme in bug 679939 comment 29 ;)

Comment 7

[Jan de Mooij [:jandem]]

I started working on this because it blocks a number of things like bug 922406 (Ion compilation of global scripts that use `this`) and general `this`-cleanup. It's also a really nice simplification.

Plan of attack is:

Step 1: Add global->outerWindow as described in comment 0. When we call the outerObject and innerObject hooks, assert the hook's return value matches the value as it'd be in the new world.

Step 2: Remove the hooks.

I did step 1 and this exposed a minor problem with this part in comment 0:

> Lastly, given all this, it seems like the innerObject hook could be replaced with the expression:
>   (obj == obj->compartment->outerWindow ? obj->compartment->global : obj)

Here we also have to check obj is still an OuterWindowProxy (we could have transplanted it with a CCW). My patch does that by storing the OuterWindowProxy Class in the Runtime.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Step 1

Just for reference, this patch implements the new system and verifies its correctness by comparing to the current hooks.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8681465 [details] [diff] [review]
Step 1

Review of attachment 8681465 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

::: js/src/jsobj.cpp
@@ +79,5 @@
> +JSObject*
> +js::GetInnerObject(JSObject* obj)
> +{
> +    JSObject* expected =
> +        (obj->getClass() == obj->runtimeFromMainThread()->maybeOuterWindowClass()) ? &obj->global() : obj;

Good point that the class could have changed.  Just to check: this wasn't for use as a scope chain, right?  (comment 4 seems to assert that shouldn't happen.)  I was wondering: even if the assertion in this patch fails and thus the new behavior doesn't precisely match the innerObject hook behavior, it seems weird that you get a different object at different times for the same 'window' object.  It's hard to think about this b/c I don't have an intuitive understanding of why we even need to innerize outside of the scope chain use case (do we?  is this old pray-and-spray security code?)

Second: the test assumes that a class match implies an object identity match which might be true now but seems kindof subtle and perhaps it could change in the future, so maybe the test should match both?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #9)
> Good point that the class could have changed.  Just to check: this wasn't
> for use as a scope chain, right?  (comment 4 seems to assert that shouldn't
> happen.)
> I was wondering: even if the assertion in this patch fails and
> thus the new behavior doesn't precisely match the innerObject hook behavior,
> it seems weird that you get a different object at different times for the
> same 'window' object.

This was for the GetInnerObject call in BaselineIC.cpp. There we're checking for `window.foo` GETPROPs, we can optimize those to go to the global object. It's important we match the innerObject behavior for that.

> Second: the test assumes that a class match implies an object identity match
> which might be true now but seems kindof subtle and perhaps it could change
> in the future, so maybe the test should match both?

Definitely! I changed this in the actual patch.

Comment 11

[Luke Wagner [:luke]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> This was for the GetInnerObject call in BaselineIC.cpp. There we're checking
> for `window.foo` GETPROPs, we can optimize those to go to the global object.
> It's important we match the innerObject behavior for that.

Ah hah, reading the use in BaselineIC.cpp actually gives me some intuition now.  Given that the scope chain use of GetInnerObject() should be removed / special-cased, I wonder if we could replace GetInnerObject() with:

  bool IsWindowProxyToCurrentGlobal(cx, obj);

which I think is a much more intuitive question (if you know what a WindowProxy is). Then the code in BaselineIC.cpp could be written:

  if (!IsWindowProxyToCurrentGlobal(cx, obj))
    return true;
  obj = cx->global();  // script->global == cx->global b/c CPG

Also, since the whole inner/outer hack is for WindowProxy, perhaps we should name the field GlobalObject::windowProxy() and have a nice beefy comment linking to
  https://html.spec.whatwg.org/multipage/browsers.html#the-windowproxy-object
which should hopefully be at least a little more intuitive to newcomers.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 1

Looking for feedback on this approach.

In particular feedback on the following renames/refactorings, because if I have to change these names I'd rather do it ASAP before it'll bitrot other patches in the stack.

js::IsOuterObject(obj) -> js::IsWindowProxy(obj)
js::IsInnerObject(obj) -> js::IsGlobalWithWindowProxy(obj)

JS_ObjectToInnerObject -> JS_WindowProxyToGlobal
JS_ObjectToOuterObject -> JS_GlobalToWindowProxy

Note that IsGlobalWithWindowProxy can't be just IsGlobal: not all globals have a WindowProxy (an outerObject hook) and they can end up in code that asserts !IsInnerObject.

Comment 13

[Jan de Mooij [:jandem]]

Btw, there are a number of places where we use the outer/inner names, especially in Gecko, so maybe I should use OuterWindowProxy instead of WindowProxy to make the connection more obvious? Maybe we want to get rid of the inner/outer names eventually, but I don't see that changing in Gecko anytime soon.

Comment 14

[Jan de Mooij [:jandem]]

Sorry for the bugspam but one last comment: what I don't really like about the name WindowProxyToGlobal is that readers may think objects passed to it *must be* WindowProxies (and similarly, anything passed to GlobalToWindowProxy must be a global object).

That's not true. Like ObjectToInnerObject and ObjectToOuterObject, these functions can be called on any object.

Not sure what's a better name though, MaybeWindowProxyToGlobal? Suggestions welcome.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 8681652 [details] [diff] [review]
Part 1

Review of attachment 8681652 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfriendapi.h
@@ +680,2 @@
>  inline bool
> +IsGlobalWithWindowProxy(JSObject* obj)

Based on below comments, I think this name isn't quite right either.

::: js/src/jsobj.cpp
@@ +110,5 @@
> +{
> +    // Note: simply checking `obj == obj->global().windowProxy()` is not
> +    // sufficient: we may have transplanted the window proxy with a CCW.
> +    // Check the Class to ensure we really have a window proxy.
> +    return obj->getClass() == obj->runtimeFromMainThread()->maybeWindowProxyClass();

I think IsWindowProxy()/*WindowProxyClass isn't quite the right name: a WindowProxy is always a WindowProxy, no matter the dynamic state of the window (whether it is window of the active document or not).  That is, global().windowProxy() always points to a WindowProxy, no matter global().windowProxy().getClass().  Scanning
  https://html.spec.whatwg.org/multipage/browsers.html#the-windowproxy-object
again for terminology, it sounds like maybe the right name is ActiveDocumentWindowProxy(Class)?  Or, taking a little liberty, ActiveWindowProxy?  I'd be interested to hear what bz thinks.

::: js/src/jsobj.h
@@ +1062,5 @@
>   * this. (Users can't simply pass the Window, because the Window isn't exposed
>   * to scripts.)
>   */
> +JSObject*
> +WindowProxyToGlobal(JSObject* obj);

You're right these names are awkward.  Even worse, based on the above comment, they don't even go from *every* WindowProxy to a global, only a WindowProxy for the active document's Window!

Comment 16

[Boris Zbarsky [:bzbarsky]]

> That is, global().windowProxy() always points to a WindowProxy

So... There is only one WindowProxy associated with the browsing context (per spec).  The fact that we have these cross-compartment wrappers is an implementation detail.  There is no separate "active document window proxy"; the whole point is that the WindowProxy is shared across all the documents, active and inactive.

So I think it makes sense for us to call the object that uses the windowproxy class the "windowproxy" and accept/document that global().whatever() (naming to be determined) is in the same compartment as global() and is either a windowproxy or a cross-compartment wrapper for one.

> js::IsOuterObject(obj) -> js::IsWindowProxy(obj)
> js::IsInnerObject(obj) -> js::IsGlobalWithWindowProxy(obj)

These look good to me.  In terms of current semantics, IsOuterObject() is precisely IsWindowProxy(), imo.  And IsInnerObject is IsGlobalWithWindowProxy.

> JS_ObjectToInnerObject -> JS_WindowProxyToGlobal

This is more like JS_ToGlobalIfWindowProxy, as you point out.

> JS_ObjectToOuterObject -> JS_GlobalToWindowProxy

This one is a bit more like JS_ToWindowProxyIfGlobal.  And even then it returns a cross-compartment wrapper for the WindowProxy...

And really, it's not going to give you a WindowProxy for non-window globals.  So maybe just JS_ToWindowProxyIfWindow?

> Btw, there are a number of places where we use the outer/inner names, especially in Gecko, 

We're working on changing that.  E.g. Kyle is doing the work to split nsGlobalWindow into two separate classes, and the XPCOM interface names he's proposing for those classes are mozIDOMWindow and mozIDOMWindowProxy.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8681652 [details] [diff] [review]
Part 1

I do agree that the fact that we have these cross-compartment wrappers so IsWindowProxy(global().windowProxy()) will test false at times is annoying.  I'm not quite sure what to do about that.

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #16)
> And really, it's not going to give you a WindowProxy for non-window globals.
> So maybe just JS_ToWindowProxyIfWindow?

Hm, if we're going to use "Window" here, we can also rename IsGlobalWithWindowProxy(obj) to IsWindow(obj), right?

So we'd have:

* IsWindowProxy(obj)
* IsWindow(obj)

* ToWindowProxyIfWindow
* ToWindowIfWindowProxy

(In reply to Boris Zbarsky [:bz] from comment #16)
> We're working on changing that.  E.g. Kyle is doing the work to split
> nsGlobalWindow into two separate classes, and the XPCOM interface names he's
> proposing for those classes are mozIDOMWindow and mozIDOMWindowProxy.

Woo, great. Then Window and WindowProxy would match nicely I think.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> we can also rename IsGlobalWithWindowProxy(obj) to IsWindow(obj), right?

Yes, so we can.

>* IsWindowProxy(obj)
>* IsWindow(obj)

Hey, this is almost sounding readable.  ;)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Oh, it might be worth seeing what Bobby thinks here too...

Comment 21

[Bobby Holley (:bholley)]

IsWindow and IsWindowProxy sound good. I don't think there should be any automatic unwrapping of CCWs in those tests - that would be be a behavior change from the current convention, and has the potential to create a lot of subtle bugs.

Comment 22

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #17)
> I do agree that the fact that we have these cross-compartment wrappers so
> IsWindowProxy(global().windowProxy()) will test false at times is annoying. 
> I'm not quite sure what to do about that.

This was my main concern above: as you said, there is only one window proxy and it doesn't become not-a-window-proxy when you navigate.  

> In terms of current semantics, IsOuterObject() is precisely IsWindowProxy(), imo.

So I wasn't able to tell b/c I don't know how the Class::innerObject hooks look on CCW-to-WindowProxy, but does IsOuterObject() behave the same way currently (it becomes false when your WindowProxy is transplanted to a CCW)?

I wonder if many of the uses of IsWindowProxy() even know about this corner case of the current IsOuterObject() and/or depend on this subtlety or if most could use the simple definition:
  IsWindowProxy(obj) { return obj == obj->global().windowProxy(); }
Jan's BaselineIC.cpp example above shows clearly there are, but I wonder if this is the minority and they could be served by a separate, explicit predicate (e.g. IsWindowProxyToCurrentGlobal).

Comment 23

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #22)
> So I wasn't able to tell b/c I don't know how the Class::innerObject hooks
> look on CCW-to-WindowProxy,

I'm pretty sure we don't have a separate JSClass for CCW-to-outer, so I think the innerObject hook is null on the CCW (per comment 21).

> but does IsOuterObject() behave the same way
> currently (it becomes false when your WindowProxy is transplanted to a CCW)?

Given that IsOuterObject is defined as |return !!GetObjectClass(obj)->ext.innerObject;|, that would be consistent with the above, and correct IMO.

Comment 24

[Jan de Mooij [:jandem]]

Attachment: Part 1 - New Window/WindowProxy mechanism

This uses IsWindow, IsWindowProxy, ToWindowIfWindowProxy and ToWindowProxyIfWindow as discussed.

Also a fair amount of cleanup and commenting compared to the previous patch.

Comment 25

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Rename stopAtOuter to stopAtWindowProxy

This renames the stopAtOuter argument to *Unwrap to stopAtWindowProxy.

Comment 26

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Use IsWindow/IsWindowProxy instead of class hook checks

Some places check whether the Class has an innerObject/outerObject hook. This patch replaces those checks with calls to IsWindow and IsWindowProxy.

Comment 27

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Don't invoke thisValue hook in GetThisValue

This fixes GetThisValue (and ComputeImplicitThis) to not invoke any Class hooks.

The signature changes from:

  bool GetThisValue(JSContext* cx, HandleObject obj, MutableHandleValue vp)

to:

  Value GetThisValue(JSObject* obj);

Pretty nice.

Unfortunately since I filed this bug, ModuleEnvironmentObject and ClonedBlockObject also got this-hooks, but I think checking for 4 Classes is still better and simpler to understand than having the hook on everything.

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8682475 [details] [diff] [review]
Part 4 - Don't invoke thisValue hook in GetThisValue

Actually one reviewer is enough for this I think; the patch isn't that complicated. (Feel free to comment, of course.)

Comment 29

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Remove innerObject/outerObject/thisValue hooks

Comment 30

[Jan de Mooij [:jandem]]

Attachment: Part 6 - rm nsGlobalWindow::OuterObject and ObjectToOuterObjectValue

nsGlobalWindow::OuterObject and ObjectToOuterObjectValue are now unused.

Comment 31

[Jan de Mooij [:jandem]]

Attachment: Part 1 - New Window/WindowProxy mechanism

Small changes to make SetWindowProxyClass take a JSRuntime* instead of JSContext*, and call it only once, when creating the runtime.

Comment 32

[Jan de Mooij [:jandem]]

Waldo doesn't like using the Window and WindowProxy names inside the JS engine. Unfortunately I can't think of good names that work for both (1) the web (different Window + WindowProxy) and (2) single global (shell etc.) cases. Ideally there'd be only (1) so that we could have just IsExposedGlobal and IsScopeGlobal without caring about (2) but that's not going to happen in this bug.

One option is this:

* IsWindowProxy -> IsGlobalProxy
* IsWindow -> IsGlobalWithGlobalProxy

* ToWindowProxyIfWindow -> ToGlobalProxyIfGlobal
* ToWindowIfWindowProxy -> ToGlobalIfGlobalProxy

However I don't know if that's better. What do you all think?

Comment 33

[Bobby Holley (:bholley)]

I think that the WindowProxy setup is so specific that it does the JS engine and other embedders a disservice to pretend it's anything more general than it is (the same was true for the "inner and outer object" setup).

For example, the "GlobalWithProxy" makes me think of the debugger proxy scope chain, as well as proxies-as-globals, neither of which are at all related.

I think we should just call it Window and WindowProxy - all of the logic that deals with those cases will be inherently web- and dom-specific anyway.

Comment 34

[Jan de Mooij [:jandem]]

Thanks Bobby. Let's just stick with the current names then so I don't have to rename everything again.

Patches are green on Try FWIW:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d140b143acbb&group_state=expanded

Comment 35

[Shu-yu Guo [:shu]]

Comment on attachment 8682475 [details] [diff] [review]
Part 4 - Don't invoke thisValue hook in GetThisValue

Review of attachment 8682475 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: js/src/jsobj.cpp
@@ +3098,5 @@
> +    if (obj->is<GlobalObject>())
> +        return ObjectValue(*ToWindowProxyIfWindow(obj));
> +
> +    if (obj->is<ClonedBlockObject>())
> +        return obj->as<ClonedBlockObject>().thisValue();

I'd be interested to see which branch is hotter, the GlobalObject one or the ClonedBlockObject one.

::: js/src/jsobj.h
@@ +1046,5 @@
>   *
>   * See comments at ComputeImplicitThis.
>   */
> +Value
> +GetThisValue(JSObject* obj);

Seeing how this function is no longer fallible, perhaps rename to just ThisValue? I can see an argument for that being confusing, however.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8682472 [details] [diff] [review]
Part 2 - Rename stopAtOuter to stopAtWindowProxy

r=me

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8682474 [details] [diff] [review]
Part 3 - Use IsWindow/IsWindowProxy instead of class hook checks

r=me

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8682489 [details] [diff] [review]
Part 6 - rm nsGlobalWindow::OuterObject and ObjectToOuterObjectValue

r=me

Comment 39

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8682544 [details] [diff] [review]
Part 1 - New Window/WindowProxy mechanism

Review of attachment 8682544 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +1101,5 @@
>    JSObject *obj = js::Wrapper::New(cx, global,
>                                     isChrome ? &nsChromeOuterWindowProxy::singleton
>                                              : &nsOuterWindowProxy::singleton,
>                                     options);
> +  MOZ_ASSERT_IF(obj, js::IsWindowProxy(obj));

Note that debug builds will crash on the next assertion if obj is null.

Comment 40

[Jan de Mooij [:jandem]]

(In reply to :Ms2ger from comment #39)
> > +  MOZ_ASSERT_IF(obj, js::IsWindowProxy(obj));
> 
> Note that debug builds will crash on the next assertion if obj is null.

Yup, part 3 removes that (old) assertion so I didn't bother fixing it :)

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8682544 [details] [diff] [review]
Part 1 - New Window/WindowProxy mechanism

>+++ b/js/src/jit/IonBuilder.cpp
>+    if (!IsWindowProxy(singleton))

This one is worth a comment.  In particular, if IsWindowProxy(singleton), then we are looking at our own WindowProxy, not some other window's, because those would be cross-compartment wrappers that would test false for IsWindowProxy.

r=me

Comment 42

[Luke Wagner [:luke]]

Comment on attachment 8682478 [details] [diff] [review]
Part 5 - Remove innerObject/outerObject/thisValue hooks

Review of attachment 8682478 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ \o/ \o/
(Sorry for delay)

Comment 43

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #35)
> I'd be interested to see which branch is hotter, the GlobalObject one or the
> ClonedBlockObject one.

I just added some logging, started the browser and opened a few websites. I got 3751 hits for the GlobalObject branch and 1003 for the ClonedBlockObject one. I think that's also because ClonedBlockObject::thisValue can recursively call GetThisValue with the global object, so we'll hit both branches in that case.

> Seeing how this function is no longer fallible, perhaps rename to just
> ThisValue? I can see an argument for that being confusing, however.

Hm yeah, I agree GetThisValue is less confusing.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5045d56439
https://hg.mozilla.org/integration/mozilla-inbound/rev/61022cd922f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/150f4e0ec3f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/07d90b29a810
https://hg.mozilla.org/integration/mozilla-inbound/rev/2954012024e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1dc3e22d19

Comment 45

[Mark Finkle (:mfinkle) (use needinfo?)]

Jan - We started to track APK size changes in treeherder. I noticed this patchset added ~30KB to the size of the APK. Looking at the patchset, I didn't notice anything that would add size. Looked like most of the changes are swapping code, not adding it.

30KB is not a lot, but I'd like to learn more about the affect of code changes. I suspect the way we get to large APK sizes is a lot of small increases.

https://treeherder.mozilla.org/perf.html#/graphs?timerange=86400&series=[mozilla-inbound,4eb0cde5431ee9aeb5eb14512ddb3da6d4702cf0,1]

Comment 46

[Jan de Mooij [:jandem]]

(In reply to Mark Finkle (:mfinkle) from comment #45)
> Jan - We started to track APK size changes in treeherder. I noticed this
> patchset added ~30KB to the size of the APK. Looking at the patchset, I
> didn't notice anything that would add size.

That's unexpected yes. It removes 3 pointers from each Class (and there are a lot of Classes), so I'd expect it to be a win. Let me check how it affects other platforms...

Comment 47

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #46)
> That's unexpected yes. It removes 3 pointers from each Class (and there are
> a lot of Classes), so I'd expect it to be a win. Let me check how it affects
> other platforms...

Win32 opt build compared to the push before it:

ZIP:       1.3 KB smaller (51601326 to 51600018 bytes)
xul.dll:  31.0 KB smaller (38116144 to 38084400 bytes)

Win64 opt build:

ZIP:       8.8 KB bigger  (54913623 to 54922681 bytes)
xul.dll:  42.5 KB smaller (50033968 to 49990448 bytes)

Linux64 opt build:

tar.bz2:    3.1 KB bigger  (55553186 to 55556451 bytes)
libxul.so: 83.1 KB smaller (99077269 to 98992193 bytes)

Linux32 opt build:

tar.bz2:    3.1 KB smaller (56511817 to 56508592 bytes)
libxul.so: 24.6 KB smaller (95131957 to 95106770 bytes)

(OS X builds still not finished because of an intermittent build failure...)

On all these platforms, libxul got smaller, as I expected. Only on Android it got bigger:

Android 4.2 x86 opt:   42.3 KB bigger (27951573 to 27994985 bytes)
Android 2.3 API9 opt:  22.4 KB bigger (23709430 to 23732321 bytes)
Android 4.0 API11+ opt:24.1 KB bigger (23704768 to 23729426 bytes)

Not sure why. Maybe a different compiler version?

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c5045d56439
https://hg.mozilla.org/mozilla-central/rev/61022cd922f8
https://hg.mozilla.org/mozilla-central/rev/150f4e0ec3f9
https://hg.mozilla.org/mozilla-central/rev/07d90b29a810
https://hg.mozilla.org/mozilla-central/rev/2954012024e1
https://hg.mozilla.org/mozilla-central/rev/ff1dc3e22d19

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c5045d56439
https://hg.mozilla.org/mozilla-central/rev/61022cd922f8
https://hg.mozilla.org/mozilla-central/rev/150f4e0ec3f9
https://hg.mozilla.org/mozilla-central/rev/07d90b29a810
https://hg.mozilla.org/mozilla-central/rev/2954012024e1
https://hg.mozilla.org/mozilla-central/rev/ff1dc3e22d19