1153266 - Allow turning on unboxed objects with an environment variable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

For testing unboxed objects in the browser on AWFY it would be good to be able to turn on unboxed objects with an environment variable.

Comment 1

[Douglas Crosher [:dougc]]

Comment on attachment 8590877 [details] [diff] [review]
patch

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

::: js/src/vm/UnboxedObject.cpp
@@ +954,5 @@
>  bool
>  js::TryConvertToUnboxedLayout(ExclusiveContext* cx, Shape* templateShape,
>                                ObjectGroup* group, PreliminaryObjectArray* objects)
>  {
> +    if (!getenv("JS_OPTION_USE_UNBOXED_OBJECTS")) {

Might it be best to cache this at startup, particularly as getenv is not required to be thread safe.

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Douglas Crosher [:dougc] from comment #1)
> Might it be best to cache this at startup, particularly as getenv is not
> required to be thread safe.

Well, I should have said this earlier but this environment variable will be removed when unboxed objects are turned on by default.  So this will really only be used for getting numbers from AWFY in the interim.  AFAICT there are other places where we use getenv in the JS engine off thread.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8590877 [details] [diff] [review]
patch

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

::: js/src/vm/UnboxedObject.cpp
@@ +957,5 @@
>  {
> +    if (!getenv("JS_OPTION_USE_UNBOXED_OBJECTS")) {
> +        if (!templateShape->runtimeFromAnyThread()->options().unboxedObjects())
> +            return true;
> +    }

Can we use JitOptions.cpp? There we have all these environment checks...

Comment 4

[Brian Hackett [Laid off!]]

JitOptions.cpp seems like it is for JIT options, and unboxed objects/arrays are not JIT things, so it seems weird to control them there.  Maybe JitOptions.cpp should be moved somewhere else and repurposed for turning on/off minor features throughout the engine?

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8590877 [details] [diff] [review]
patch

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

Good point

::: js/src/vm/UnboxedObject.cpp
@@ +954,5 @@
>  bool
>  js::TryConvertToUnboxedLayout(ExclusiveContext* cx, Shape* templateShape,
>                                ObjectGroup* group, PreliminaryObjectArray* objects)
>  {
> +    if (!getenv("JS_OPTION_USE_UNBOXED_OBJECTS")) {

Can you add a comment that this is temporary?
Maybe also eagerly return when NIGHTLY_BUILD is not set?

Comment 6

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40723ea7e393

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40723ea7e393

Comment 8

[Hannes Verschore [:h4writer]]

Enabled on Windows 8 and android firefox slave:
http://arewefastyet.com/#machine=27
http://arewefastyet.com/#machine=27

If possible could you fix the kraken crashes? The AWFY slaves use a relative long timer to detect these cases, before going to the next benchmark.

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Hannes Verschore [:h4writer] from comment #8)
> Enabled on Windows 8 and android firefox slave:
> http://arewefastyet.com/#machine=27
> http://arewefastyet.com/#machine=27
> 
> If possible could you fix the kraken crashes? The AWFY slaves use a relative
> long timer to detect these cases, before going to the next benchmark.

Bug 1155033 should fix these crashes.

Comment 10

[Shu-yu Guo [:shu]]

I noticed while reading code today that unboxed arrays are still Nightly-only. Should we let it ride the trains now?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #10)
> I noticed while reading code today that unboxed arrays are still
> Nightly-only. Should we let it ride the trains now?

Unboxed arrays don't pass tests atm and have some perf issues we would have to fix first..

Comment 12

[Brian Hackett [Laid off!]]

Yeah, it would be nice to turn on unboxed arrays but I kind of ran out of steam working on benchmark optimization, especially after bug 1162272.