1374934 - Could js::NativeGetProperty be faster?

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[(no longer active)]

This shows up as pretty heavy.  Under perf, shows up as 2.38% of total time running the full Speedometer test suite (from a profile of about 100 or so tests.)

-    2.38%     1.11%  Web Content      libxul.so                   [.] js::NativeGetProperty
   - 1.27% js::NativeGetProperty                                                            
      - 1.09% js::CallGetter                                                                
           1.00% js::InternalCallOrConstruct                                                
   - 0.55% 0x4200796172724100                                                               
        0x8000000                                                                           

Note the high self time (1.11% *total time*.)

Comment 1

[(no longer active)]

In my profile it seems like around 35-40% of the self time is spent in ShapeTable::searchUnchecked() that is inlined here from <https://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/js/src/vm/NativeObject-inl.h#744>.

Comment 2

[Jan de Mooij [:jandem]]

There are a few minor things to fix, but nothing very significant we can optimize in NativeGetProperty itself. It would be more interesting to look at the callers - could be the interpreter, ICs failing to optimize, a builtin function doing a property lookup, etc.

I can add some logging to see which properties we're looking up here.

Comment 3

[Jan de Mooij [:jandem]]

I'll try to post an initial breakdown of this tomorrow, then we know better what/where to optimize.

Comment 4

[(no longer active)]

Attachment: stats gathering patch

I collected some data about this yesterday and today, not sure how useful my findings are, here is a summary.

Firstly, not all frameworks tend to call NativeGetProperty that heavily.  It seems that the following frameworks are the ones that utilize it more heavily than others:
  * EmberJS
  * BackboneJS
  * AngularJS
  * jQuery
  * Flight

Ember and Angular's use looks particularly heavy.

I applied the following patch to gather some stats on what types of properties are observed in this function.  Here is the output per each framework above after 10 runs from InteractiveRunner.html:

EmberJS
Currently observed: 0.999973 undefined, 0.975372 null, 0.975372 notobj, 0.000000 hasbuiltin, 0.975264 assignedbuiltin, 0.975372 hasinteresting, 0.000108 nobuiltintag, 0.000000 fullydynamic
BackboneJS
Currently observed: 0.969224 undefined, 0.969224 null, 0.969224 notobj, 0.000000 hasbuiltin, 0.876056 assignedbuiltin, 0.969224 hasinteresting, 0.093168 nobuiltintag, 0.000000 fullydynamic
AngularJS
Currently observed: 1.000000 undefined, 0.875870 null, 0.875870 notobj, 0.000000 hasbuiltin, 0.875865 assignedbuiltin, 0.875870 hasinteresting, 0.000004 nobuiltintag, 0.000000 fullydynamic
jQuery
Currently observed: 0.999881 undefined, 0.999881 null, 0.999881 notobj, 0.000000 hasbuiltin, 0.999881 assignedbuiltin, 0.999881 hasinteresting, 0.000000 nobuiltintag, 0.000000 fullydynamic
Flight
Currently observed: 0.996667 undefined, 0.996667 null, 0.996667 notobj, 0.000000 hasbuiltin, 0.955514 assignedbuiltin, 0.996667 hasinteresting, 0.041152 nobuiltintag, 0.000000 fullydynamic

Some interesting observations:
 * The "nobuildintag" case seems to be for JS objects wrapping DOM nodes, where we call GetObjectClassName() to get the JSClass name (which turns out something like "NodeList" or "HTMLLiElement" or some such) and we construct the resulting atom over and over again.  This is kind of wasteful.  Have we considered creating some set of static atoms like "[object NodeList]" and "[object HTMLLiElement]"?
 * We also encounter a fair number of JSClass's with name "Object", it'd be nice to not have to create "[object Object]" atoms for those every single time also.
 * It seems like in Angular and Backbone, in a relatively high number of cases (~13%) GetInterestingSymbolProperty() returns false.  It may be interesting to discover why that is, and whether we can discover the condition earlier and bail out sooner.

Comment 5

[Jan de Mooij [:jandem]]

Ehsan, these measurements are for obj_toString right? The Ember case is bug 1384562. See also bug 1385215, although that won't work in all cases but it's a start.

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #4)
>  * We also encounter a fair number of JSClass's with name "Object", it'd be
> nice to not have to create "[object Object]" atoms for those every single
> time also.

Hm we optimize this for the (Unboxed)PlainObject classes, you mean there are other classes that show up a lot that also have clasp->name == "Object"? It might make sense to add a HashMap<Class*, JSAtom*> cache so we avoid the StringBuffer overhead and don't have to go through AtomizeString each time. Not sure how much it would help but worth measuring.

>  * It seems like in Angular and Backbone, in a relatively high number of
> cases (~13%) GetInterestingSymbolProperty() returns false.  It may be
> interesting to discover why that is, and whether we can discover the
> condition earlier and bail out sooner.

Are you sure that's GetInterestingSymbolProperty? It means we threw an exception when looking up Symbol.toStringTag, which is pretty unexpected.

Probably a good idea to file a generic obj_toString bug to track all of these things :)

Comment 6

[Jan de Mooij [:jandem]]

I looked a bit at the JS callers of NativeGetPropertyInline:

* #1 (1691953 hits) is the join call under ArrayToString, this is most likely the notorious array-stringification code in Ember.js where we have to get the toString property etc.

* Then #2 is the "get" function in Ember with 122611 hits. Not sure if we can use our megamorphic stubs for this, need to investigate more. There are some other highly-polymorphic ones like this.

* JSON.stringify shows up a lot because we do property lookups under it. Maybe we could optimize them better somehow.

* We need a megamorphic stub for unboxed object properties. Should be pretty easy and should hopefully make at least 40,000 lookups faster on the Inferno benchmark.

* Flight uses a scripted proxy object. See this[b] in getItem in memorystorage.js. We should start thinking of ways to optimize this. About 41406 hits.

* About 10355 lookups under RegExpGetSubstitution in RegExpGetComplexReplacement in self-hosted JS.

* 3965 hits on AngularJS on this line:

  if (element instanceof JQLite) {

|element| here is everything from Window to random HTML elements. If this is |.prototype| lookup on JQLite we should be able to do better... There's another instanceof on React that seems similar.

Overall we're doing much better than I expected. I think CacheIR has been very effective at eliminating these calls. I'll file some follow-up bugs.

Comment 7

[Jan de Mooij [:jandem]]

To be clear, the list in comment 6 is not exhaustive, but there aren't many other locations with more than 5000 hits.

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> To be clear, the list in comment 6 is not exhaustive, but there aren't many
> other locations with more than 5000 hits.

Do you know what the total number of calls to NativeGetPropertyInline is?  (I'm wondering how long the tail is in the distribution here, and what percentage of calls would be fixed by #1.)

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #8)
> Do you know what the total number of calls to NativeGetPropertyInline is? 
> (I'm wondering how long the tail is in the distribution here, and what
> percentage of calls would be fixed by #1.)

About 3.0-3.2 million calls (this includes Firefox chrome JS!). So yeah #1 is more than 50% of that, which is pretty ridiculous.

There is a long tail due to property lookups in the interpreter. GetProps in the JITs are optimized very well nowadays, the main issue is lookups in the VM (under ToPrimitive, JSON.stringify, etc). 

If you're interested in #1, see also bug 1384562.

Comment 10

[Tom S [:evilpie]]

Maybe we should add some primitive IC mechanism back to the VM again? Seems like Chakra has something like that.

Comment 11

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals