Avoid some "critical" #includes in and around SpiderMonkey

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Attachment 789006 [details] from bug 901132 has a list of #include statements listed by "criticality".  For each #include statement, if it were removed, the number indicates (approximately) how many fewer times we would read the corresponding header during a browser build.

I have some patches that remove some of the critical #includes, which are:

- 2891 js/public/CharacterEncoding.h from js/src/jsapi.h 
- 2136 mfbt/ThreadLocal.h from js/src/jsapi.h 
- 173 js/src/yarr/YarrSyntaxChecker.h from js/src/vm/RegExpObject.h 
- 148 --GENERATED--/js/src/jsautooplen.h from js/src/vm/Stack.h
-  73 js/src/vm/String-inl.h from js/src/gc/Barrier-inl.h

Plus a couple of other patches that remove unnecessary #includes that I had lying around.

With these, I see a 1.5% speed-up when building SpiderMonkey.  But there will also be effects outside SpiderMonkey by removing the first two cases in the list above.

I have a similar analysis script that works just within SpiderMonkey.  Here are some notable changes in how often some headers are included during a JS shell build, once these patches are applied:

vm/MatchPairs.h:          176 --> 7
vm/String-inl.h:          112 --> 10
yarr/YarrParser.h:        178 --> 3
yarr/YarrSyntaxChecker.h: 177 --> 2
(Assignee)

Comment 1

4 years ago
Created attachment 789948 [details] [diff] [review]
(part 1) - Don't #include js/CharacterEncoding.h in jsapi.h
Attachment #789948 - Flags: review?(luke)
(Assignee)

Comment 2

4 years ago
Created attachment 789949 [details] [diff] [review]
(part 2) - Don't #include mozilla/ThreadLocal.h in jsapi.h.
Attachment #789949 - Flags: review?(luke)
(Assignee)

Comment 3

4 years ago
Created attachment 789950 [details] [diff] [review]
(part 3) - Minimize #includes in vm/RegExpObject.h.
Attachment #789950 - Flags: review?(luke)
(Assignee)

Comment 4

4 years ago
Created attachment 789951 [details] [diff] [review]
(part 4) - Don't #include jsautooplen.h in vm/Stack.h.
Attachment #789951 - Flags: review?(luke)
(Assignee)

Comment 5

4 years ago
Created attachment 789952 [details] [diff] [review]
(part 5) - Don't #include vm/String-inl.h in gc/Barrier-inl.h.
Attachment #789952 - Flags: review?(luke)
(Assignee)

Comment 6

4 years ago
Created attachment 789953 [details] [diff] [review]
(part 5) - Don't #include jsprf.h in three places it isn't needed.
Attachment #789953 - Flags: review?(luke)
Comment on attachment 789951 [details] [diff] [review]
(part 4) - Don't #include jsautooplen.h in vm/Stack.h.

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

::: js/src/vm/Stack.cpp
@@ +450,5 @@
>  }
>  
>  /*****************************************************************************/
>  
> +// Unlike the other methods of this calss, this method is defined here so that

class

Updated

4 years ago
Attachment #789948 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #789949 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #789950 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #789951 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #789952 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #789953 - Flags: review?(luke) → review+
(Assignee)

Comment 8

4 years ago
Comment on attachment 789952 [details] [diff] [review]
(part 5) - Don't #include vm/String-inl.h in gc/Barrier-inl.h.

> (part 5) - Don't #include vm/String-inl.h in gc/Barrier-inl.h.

Try server tells me that this patch breaks the world, so I won't include it when I land the others.
Attachment #789952 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #789953 - Attachment description: (part 6) - Don't #include jsprf.h in three places it isn't needed. → (part 5) - Don't #include jsprf.h in three places it isn't needed.
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c084ee924106
https://hg.mozilla.org/integration/mozilla-inbound/rev/71aaddc68ce0
https://hg.mozilla.org/integration/mozilla-inbound/rev/621142a4590c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea28db5b5cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/82a32598ad71
https://hg.mozilla.org/mozilla-central/rev/c084ee924106
https://hg.mozilla.org/mozilla-central/rev/71aaddc68ce0
https://hg.mozilla.org/mozilla-central/rev/621142a4590c
https://hg.mozilla.org/mozilla-central/rev/0ea28db5b5cb
https://hg.mozilla.org/mozilla-central/rev/82a32598ad71
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.