1155754 - Remove JSOP_OBJECT

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

JSOP_OBJECT was originally designed to optimize JS-evals containing json-like data (e.g. "eval('{...}')").  However, that particular use case has been handled by having eval directly check for json-like data and using the JSON parser in those situations.

This leaves JSOP_OBJECT in a bit of a bind.  It only applies in contexts where scripts are executed exactly once (i.e. top-level scripts), which are places where object allocation optimization should not be that relevant or important.

Testing the scores between using JSOP_OBJECT and not using it, on octane reveals that the score differences are marginal and neglible, about a 0.1% difference in score.

JSOP_OBJECT also stands in the way of eventual work to move parsing to a background thread.

Given the situation, it makes sense to remove this optimization and simplify the parser.

Comment 1

[Kannan Vijayan [:djvj]]

Octane scores.  First half are the reference scores (with JSOP_OBJECT), and the second half are scores with the parser modified to never emit JSOP_OBJECT.

python analyze.py REF_SCORES
            Richards: 24481.31 dev 6.25
           DeltaBlue: 38687.85 dev 32.79
              Crypto: 24967.56 dev 17.41
            RayTrace: 86348.86 dev 60.57
         EarleyBoyer: 31728.26 dev 10.42
              RegExp:  3031.06 dev 3.81
               Splay: 17567.72 dev 48.13
        SplayLatency: 20761.10 dev 16.13
        NavierStokes: 32577.63 dev 6.45
               PdfJS: 15211.81 dev 27.62
            Mandreel: 24544.40 dev 14.18
     MandreelLatency: 26272.66 dev 109.93
             Gameboy: 51646.54 dev 87.94
            CodeLoad: 16352.53 dev 5.04
               Box2D: 40329.80 dev 22.11
                zlib: 71037.47 dev 38.14
          Typescript: 26456.28 dev 47.35
   Score (version 9): 26389.22 dev 7.29

python analyze.py TEST_SCORES
            Richards: 24688.81 dev 17.86
           DeltaBlue: 38714.36 dev 28.82
              Crypto: 24920.07 dev 17.53
            RayTrace: 85775.73 dev 52.09
         EarleyBoyer: 31801.92 dev 11.56
              RegExp:  3031.84 dev 3.43
               Splay: 17457.13 dev 54.86
        SplayLatency: 20620.90 dev 19.81
        NavierStokes: 32581.12 dev 7.04
               PdfJS: 14989.47 dev 35.94
            Mandreel: 24471.38 dev 29.61
     MandreelLatency: 26724.25 dev 148.88
             Gameboy: 51165.35 dev 117.15
            CodeLoad: 16334.82 dev 5.60
               Box2D: 40396.87 dev 22.15
                zlib: 71023.56 dev 38.78
          Typescript: 26519.67 dev 52.05
   Score (version 9): 26359.28 dev 9.25

Comment 2

[Boris Zbarsky [:bzbarsky]]

And then we can try to remove the runonce thing?

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: remove-jsop-object.patch

Removing JSOP_OBJECT and associated things entirely.  Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02712f7202da

Comment 4

[Boris Zbarsky [:bzbarsky]]

There's more code that can go in XDRScript, I would think.  In particular, CompartmentOptions::cloneSingletons() is not useful anymore, right?  And the treatAsRunOnce() callsite there can presumably also go away, together with that comment....

Comment 5

[Kannan Vijayan [:djvj]]

Is JSOP_OBJECT the only singletonny thing that gets handled by those checks?

Comment 6

[Kannan Vijayan [:djvj]]

Man, it seems so.  This little optimization has quite the reach :)  Removed it all and am just running jit-tests right now and everything looks clean.

One thing to note about this change is that it does change the layout of objects created from literals.  Previously, the objects would be directly initialized from a Name/Value property list, and that so the object would have its shape created alongside with instantiation.  This meant that with JSOP_OBJECT usage, even index properties would get shapes.

The new scheme starts off with empty objects and initializes properties through the interpreter, which leads to the object layout being more "regular" (i.e numeric properties go into the elements array, and regular properties go into the slots array).  In the JSOP_OBJECT case, they are generally directly present on the object and the object sizes for different literals show more variation.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Is DeepCloneObjectLiteral dead code now too?

Comment 8

[Tom S [:evilpie]]

Might this degrade performance for JSONP like uses? i.e. doing something like <script>callback({a: 1})</script>

Comment 9

[Kannan Vijayan [:djvj]]

(In reply to Tom Schuster [:evilpie] from comment #8)
> Might this degrade performance for JSONP like uses? i.e. doing something
> like <script>callback({a: 1})</script>

I can't imagine that these are hot paths.  Consider that every single invocation of these are already going to go through parse, bytecode generation, and interpreter execution.. it can't be the case that their hotness matters all that much.  I'm guessing this idiom shows up most commonly in AJAX requests which use <script> content injection as a mechanism of invoking code.. but once again, that should be a slowpath already.

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: remove-jsop-object.patch

New patch, with 'cloneSingletons' methods additionally removed (over the previous patch).  Try seems green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=136998de3166

The 'X' failures on the try there are from the underlying revision, not my changes.

Comment 11

[Luke Wagner [:luke]]

In addition to DeepCloneLiteral as bz mentioned, I think you can also remove the PNK_OBJECT case of ParseNode::getConstantValue.

Removing this feature seems like a good idea given the general problems we've had recently.  I'm not sure if I'm the right reviewer, though, given neither authorship nor ownership.

The JSONP argument does seem like a valid argument, though: iiuc, JSONP is a hack to work around normal content cross-origin limitations so I think we *would* expect sizable JSONP scripts of the form Tom mentioned.  That being said, I think a better strategy would be to (1) extend the JSON hack to accept JSONP (2) then apply the JSON hack to JS::Evaluate.  Alternatively, if we were worried about true mixed content, we could do the JSOP_JSON thing already discussed.  In either case, I think we should wait until we have some motivating concrete examples to optimize and proceed with JSOP_OBJECT-removal.

Comment 12

[Kannan Vijayan [:djvj]]

A more general alternative would be what you suggested to me in chat the other day: to modify JSOP_OBJECT to take a single uint32_t offset into the source, and have it invoke a custom re-parse and materialization of the object.

The reparse code can be a lot simpler than the general parser, given that the main parser can choose to only emit JSOP_OBJECT on well-structured object literals (no weird chars in names or values, etc.)

Comment 13

[Kannan Vijayan [:djvj]]

Who is a good reviewer for this?

Comment 14

[Tom S [:evilpie]]

I mentioned the JSONP bit, because JSC seems to have exactly that optimization Luke describes. It's imaginable we never had problems in this area, because JSOP_OBJECT was fast enough. (At least I don't remember hearing about an issue with such code) https://news.ycombinator.com/item?id=9401189

Comment 15

[Luke Wagner [:luke]]

(In reply to Kannan Vijayan [:djvj] from comment #13)
jorendorff or bhackett?

(In reply to Tom Schuster [:evilpie] from comment #14)
Thanks for pointing that out!  Perhaps this is worth a bit of investigation to see how prevalent/large JSONP is (one idea is by building an instrumenting JSC browser to see how often their opt triggers).

Comment 16

[Brian Hackett [Laid off!]]

Comment on attachment 8594843 [details] [diff] [review]
remove-jsop-object.patch

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2090,1 @@
>      return true;

This function's structure can be simplified now.

::: js/src/vm/Opcodes.h
@@ -748,5 @@
> -     *   Type: Object
> -     *   Operands: uint32_t objectIndex
> -     *   Stack: => obj
> -     */ \
> -    macro(JSOP_OBJECT,    80, "object",     NULL,         5,  0,  1,  JOF_OBJECT) \

I think it would be better to add a JSOP_UNUSED rather than bump the number in every other bytecode.

Comment 17

[Kannan Vijayan [:djvj]]

(In reply to Tom Schuster [:evilpie] from comment #14)
> I mentioned the JSONP bit, because JSC seems to have exactly that
> optimization Luke describes. It's imaginable we never had problems in this
> area, because JSOP_OBJECT was fast enough. (At least I don't remember
> hearing about an issue with such code)
> https://news.ycombinator.com/item?id=9401189

As per your and Luke's suggestion, I've instrumented a build of WebKit + JSC to record and print out info on how often the JSONP special-case path is taken in the browser.

I tested against CNN and Facebook yesterday (but I only instrumented for counts, and I'm going to go test again after getting it to print average lengths)..

Do you have any idea about important sites that might be heavier on the JSONP usage?

Comment 18

[Tom S [:evilpie]]

I asked Oliver (JSC dev) this over twitter and he can't remember any either. https://twitter.com/ohunt/status/591720520216813568 (There are some more, but I couldn't figure out how to link all of his replies)

Comment 19

[Kannan Vijayan [:djvj]]

Well, I went ahead and just tested a bunch of sites on the Alexa 500, browsed them for a while using as many functions and exploring as many parts of the site as I could (baidu was hard).

in google.com, I tested the main page, search, second and third pages of search, image search, and scrolling on image search.

On facebook.com I logged in and went through main page, scrolling past infinite scroll several times, looked at posted images, viewed the friends list, and went through messages, and through profile edits.

On youtube.com I viewed a few videos and browsed through comments.

On baidu.com I went through a large number of the site's different sections, which I can only guess at since I don't read chinese (I copy/pasted text to do searches).  I went through web search, images, news, shopping, music, and maps.


Google properties used this the least.  On google.com, the optimization hit 3 times, with an average 82.33 bytes per payload.  Youtube.com hit it once, for a payload of 29 bytes.

On facebook.com, there were 80 hits across a 5-minute browsing session, with an average of 93.70 bytes, and a maximum of 1288 bytes.

Baidu.com had the worst case.  There were 54 hits across a 5-minute browsing session, with an average length of 5kb.  There was one extreme case of an objec-literal of 127kb in length.

Comment 20

[Kannan Vijayan [:djvj]]

So, given the situation, I think Tom's concern is more relevant.  If a major search site is commonly hitting object-literal sizes of 127kb, running that through bytecode-generation, then an interpreter switch loop for execution, and then the VM code for object construction.. it's going to be a perf problem.