Closed Bug 908050 (minimize-jsapi) Opened 6 years ago Closed 6 years ago

Minimize number of files that depend on jsapi.h

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:t])

I started minimizing the number of files that depend on jsapi.h in bug 905017.  But it's a big task, so here's a tracking bug.
Depends on: 903283
I'm going to track two metrics as I land patches.

- "rebuilt" is the number of files that get rebuilt after touching jsapi.h.
  This is the real metric of interest.

- "includes" is the number of times jsapi.h gets included during a build,
  assuming no only-load-a-header-once optimization.  (I determine this by
  adding a |#warning| statement to the top of jsapi.h before building, which
  defeats said optimization.)

Because jsapi.h can be transitively included many times per .cpp file, often a
single patch will affect "includes" significantly without affecting "rebuilt"
significantly.

The changes due to bug 905017 are as follows.

                landed as               rebuilt     includes
starting point  143783:493cd772def7     2631        89802
905017-part-1   143784:dba0f0715b18     2522        40146
                ...
905017-part-2   143793:7e6fb33fdf22     2500        38291
905017-part-3   143794:c85332df4320     2222        37471
Depends on: 909090
Alias: minimize-jsapi
Depends on: 909178
Depends on: 909597
Depends on: 909623
After bug 909178:

>                 landed as               rebuilt     includes
> starting point  143783:493cd772def7     2631        89802
> 905017-part-1   143784:dba0f0715b18     2522        40146
>                 ...
> 905017-part-2   143793:7e6fb33fdf22     2500        38291
> 905017-part-3   143794:c85332df4320     2222        37471
>                 ...
> 909178-part-1   144434:7f8e99aec954     1563        29371
> 909178-part-2   144435:f8fec7c369d1     1563        29383

Looks like the work in other bugs is having an effect.  Cool.
Depends on: 910109
After bug 909597:

>                 landed as               rebuilt     includes
> starting point  143783:493cd772def7     2631        89802
> 905017-part-1   143784:dba0f0715b18     2522        40146
>                 ...
> 905017-part-2   143793:7e6fb33fdf22     2500        38291
> 905017-part-3   143794:c85332df4320     2222        37471
>                 ...
> 909178-part-1   144434:7f8e99aec954     1563        29371
> 909178-part-2   144435:f8fec7c369d1     1563        29383
>                 ...
> (predecessor)   144620:3e064e6246aa     2157        34618
>                 ...
> 909597-part-2   144622:2a3ed74a0c95     2157        33801

So bug 909597's effect was minimal.

I don't know what happened between r144435 and 1444620 :(
Depends on: 908576
I did some bisecting...

> > 905017-part-3   143794:c85332df4320     2222        37471
> >                 ...
> > 909178-part-1   144434:7f8e99aec954     1563        29371
> 
> Looks like the work in other bugs is having an effect.  Cool.

That was due to patch 7 in bug 908576, courtesy of bz.

> > 909178-part-2   144435:f8fec7c369d1     1563        29383
> >                 ...
> > (predecessor)   144620:3e064e6246aa     2157        34618
> 
> I don't know what happened between r144435 and 1444620 :(

That was due to bug 909514, the patch for which accidentally adding |#include "jsapi.h"| to BindingDeclarations.h.  I landed a follow-up patch that removed it again.
Current status:

>                 landed as            rebuilt  includes
> (predecessor)   143783:493cd772def7  2631     89802
> 905017-part-1   143784:dba0f0715b18  2522     40146  minimize js header inclusions
>                 ...
> 905017-part-2   143793:7e6fb33fdf22  2500     38291  create js/StructuredClone.h
> 905017-part-3   143794:c85332df4320  2222     37471  create js/ProfilingStack.h
>
> (predecessor)                        2223     34759
> 908576-part-7   144195:08e3075e7320  1558     29303  move DOMProxyHandler classes
>                 ...
> 909178-part-1   144434:7f8e99aec954  1563     29371  create js/Id.h
> 909178-part-2   144435:f8fec7c369d1  1563     29383  do-jsclass
>                 ...
> (predecessor)   144523               1563     29376
> 909514          144524               2156     34589  add jsapi to BindingDeclarations.h
>
> (predecessor)   144620:3e064e6246aa  2157     34618
>                 ...
> 909597-part-2   144622:2a3ed74a0c95  2157     33801  do-jsdbgapi
>
> (predecessor)   144867:9b1af0e6afd4  2146     29563
> 910109-part-1   144868:73bad4a03382  2146     31302  do-jsproxy
> 910109-part-2   144869:d7f48284d66a  2146     28937  do-jswrapper
> 909514-followup 144870:fc2901f48146  1539     23778  rm jsapi from BindingDeclarations.h
Depends on: 909514
jcranmer updated the data in bug 901132... between August 4th and August 30th, the number of static transitive includes of jsapi.h dropped from 3434 to 1792.
Depends on: 912411
No longer depends on: 909090
After bug 912411:

>                 landed as            rebuilt  includes
> (predecessor)   143783:493cd772def7  2631     89802
> 905017-part-1   143784:dba0f0715b18  2522     40146  minimize js header inclusions
>                 ...
> 905017-part-2   143793:7e6fb33fdf22  2500     38291  create js/StructuredClone.h
> 905017-part-3   143794:c85332df4320  2222     37471  create js/ProfilingStack.h
>                 ...
> (predecessor)                        2223     34759
> 908576-part-7   144195:08e3075e7320  1558     29303  move DOMProxyHandler classes
>                 ...
> 909178-part-1   144434:7f8e99aec954  1563     29371  create js/Id.h
> 909178-part-2   144435:f8fec7c369d1  1563     29383  do-jsclass
>                 ...
> (predecessor)   144523               1563     29376
> 909514          144524               2156     34589  add jsapi to BindingDeclarations.h
>                 ...
> (predecessor)   144620:3e064e6246aa  2157     34618
>                 ...
> 909597-part-2   144622:2a3ed74a0c95  2157     33801  do-jsdbgapi
>                 ...
> (predecessor)   144867:9b1af0e6afd4  2146     29563
> 910109-part-1   144868:73bad4a03382  2146     31302  do-jsproxy
> 910109-part-2   144869:d7f48284d66a  2146     28937  do-jswrapper
> 909514-followup 144870:fc2901f48146  1539     23778  rm jsapi from BindingDeclarations.h
>                 ...
> (predecessor)   145772:55cdc3eef815  1533     20314 
> 912411-part-1   145773:5414483004cf  1487     19350  rm some jsapis from Gecko
> 912411-part-2   145774:053f6a2c5e29  1487     19158  rm jsapi from AccessCheck
> 912411-part-3   145775:c1262e2db291  1484     15396  rm some jsapis within SM
Depends on: 918330
After bug 918330:

>                 ...
> (njn moved to a new machine)
>                 ...
> (predecessor>   147955:11feb444914d  1510     16487
> 918330          147956:08d8559d2493  1294     15739  GfxInfoCollector.h
Currently we have just under 1300 files rebuilt when jsapi.h is touched.  To
get that down by more than a handful, we need to make the following files not
depend on jsapi.h.

- nsCxPusher.h: Uses JSAutoRequest and JSAutoCompartment, which can be moved
  from jsapi.h into new headers.

- xpcpublic.h: Uses JS_NewExternalString() and JS_GetEmptyStringValue().
  Probably not hard to remedy, though I haven't done it.

- DOMJSProxyHandler.h: uses JSPropertyDescriptor, which can be moved from
  jsapi.h into a new header.

- Workers.h: uses JSGCParamKey, which can be moved from jsapi.h into a new
  header.

- BindingUtils.h:  This is the hard one -- it uses JSObjectIsDate(),
  JS_ObjectIsRegExp(), JS_WrapValue(), JS_IsExceptionPending(),
  JS_ValueToString(), JS_GetStringCharsAndLength(), JS_InternString(),
  INTERNED_STRING_TO_JSID(), JS_GetSringCharsZAndLength(),
  JSID_TO_FLAT_STRING(), JS_FlatStringEqualsAscii(), JS::AutoIdVector,
  JS_IsNativeFunction(), and CustomAutoRooter.

- CallbackFunction.h:  JS_ObjectIsCallable() (in assertion) -- easy.
  This drops the number of rebuilt files from 1269 to 1080.

- TypedArray.h:  Uses CustomAutoRooter (like BindingUtils.h).  This drops the 
  rebuild count to 977.  

And at this point, the majority of jsapi.h includers at build-time 525 are 
.cpp files, with another 200-odd in SpiderMonkey itself.
In other words, if we perform some heroics, we can get it down from just under 1300 to just under 1000.  Probably not worth it.  I'm tempted to mark this as fixed -- it was over 2600 when this bug was filed.
Yeah, I agree, there is probably not a lot of low hanging fruit to be picked here any more...
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Part 3 of bug 907789 reduced the number of files rebuilt after touching jsapi.h from 1299 to 890.  AIUI, that's because lots of the dom bindings files are now combined, and so although the number of files is down, the average size of those files is up, and in the end it's not much different in terms of compile time.  Is that right?
(In reply to Nicholas Nethercote [:njn] from comment #12)
> Part 3 of bug 907789 reduced the number of files rebuilt after touching
> jsapi.h from 1299 to 890.  AIUI, that's because lots of the dom bindings
> files are now combined, and so although the number of files is down, the
> average size of those files is up, and in the end it's not much different in
> terms of compile time.  Is that right?

The number of files is down, the average size is up, but it was a significant win in total compile time when I measured it locally.  I think bz has also mentioned how much more pleasant full rebuilds of dom/bindings are with bug 907789.  But I am willing to believe the possibility of it being slower.
> The number of files is down, the average size is up, but it was a
> significant win in total compile time when I measured it locally.

I just measured and you're right.  Before, rebuilding after touching jsapi.h took 5m58s;  after, it took 4m53s.  Cool!
You need to log in before you can comment on or make changes to this bug.