Closed Bug 419969 Opened 12 years ago Closed 12 years ago

Lone js_Interpret or splitting jsinterp.c

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(1 file, 7 obsolete files)

It would be nice to split jsinterp.c into 2 files where the first contains just js_Interpret and the second contains the rest of functions.

In this way it would be possible to apply specific optimizations to js_Interpret like -O3 while keeping the rest of code at -Os. It would also allow to align js_Interpret on a page boundary (GCC has -falign-functions=4096 for that). 

Such alignment should help with better utilization of TLB cache entries in the interpreter. It should also reduce the benchmarking noise caused by changing js_Interpret alignment relative to cache lines when adding/deleting code in other parts of SpiderMonkey.
Attached patch prototype implementation (obsolete) — Splinter Review
Patch prototype that keeps just js_Interpret in jsintrerp.c and moves the rest of functions to jsintkit.c.

The patch adds explicit -falign-functions=4096 -O3 options so it would not compile with MSVC.
Attached file sunspider results with the patch (obsolete) —
Apparently using -falign-functions=4096 -O3 just for js_Interpret speedups the benchmark by 7%.
Attached patch v 0.2 (obsolete) — Splinter Review
Now this quilt patch includes all files.
Attachment #306137 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
This is a minimal patch. It does not add support for a special options in Makefiles for jsinterp.c. Rather it just split jsinterp.c into 2 files, jsinterp.c with js_Interpret and jsintkit.c with anything else. I will file a separated bug about this. 

Moreover, the patch does not moves any code to jsintkit.c. Rather the file includes jsinterp.c with a special define and the code in jsinterp.c decides based on the define what to define. In this way there should be very little merge conflicts with any patches that targets the interpreter. When things are stabilized, then the code can move.
Attachment #306226 - Flags: review?(brendan)
New patch coming based on IRC discussion (jsinvoke.c and jsinterp.c)?

/be
Attached patch v2 (obsolete) — Splinter Review
The new patch makes sure that js/scr compiles when JS_OPMETER is defined.
Attachment #306141 - Attachment is obsolete: true
Attachment #306226 - Attachment is obsolete: true
Attachment #306741 - Flags: review?(brendan)
Attachment #306226 - Flags: review?(brendan)
Need updated patch, but r+a=me in advance based on the last one. Need to get this in soon, and let everyone undo conflicts. Might be best to have a handshake so if one has outstanding changes to jsinterp.c, they can be merged properly.

Thanks for keeping JS_OPMETER working!

/be
Flags: blocking1.9+
Priority: -- → P1
Target Milestone: --- → mozilla1.9
Comment on attachment 306741 [details] [diff] [review]
v2

>+#ifdef js_invoke_c__
...
>@@ -6974,8 +6994,10 @@ interrupt:
> 
>         ASSERT_SAVED_SP_AND_PC(fp);
>         printable = js_AtomToPrintableString(cx, atom);
>         if (printable)
>             js_ReportIsNotDefined(cx, printable);
>         goto error;
>     }
> }
>+
>+#endif

Please comment this as usual with the .h idempotent-include guards.

If we can decide always to compile jsinterp.c and jsinvoke.c separately, then we don't need this and can simplify the patch a bit. Possible/desirable?

Again r/a+ carry over with nits and simplification, main thing is to get this in so we can start focusing on interpreter optimizations.

/be
Attachment #306741 - Flags: review?(brendan)
Attachment #306741 - Flags: review+
Attachment #306741 - Flags: approval1.9+
(In reply to comment #8)
> If we can decide always to compile jsinterp.c and jsinvoke.c separately, then
> we don't need this and can simplify the patch a bit. Possible/desirable?

I have not moved the code from jsinterp.c to avoid merging conflicts and minimize the possibility of regressions. This can be done in a separated bug.
Attached patch v3 (obsolete) — Splinter Review
The new version syncs the patch with trunk and adds the guarding comment:

#endif /* !defined js_invoke_c__ */
Attachment #306138 - Attachment is obsolete: true
Attachment #306741 - Attachment is obsolete: true
Attachment #307131 - Flags: review+
Attachment #307131 - Flags: approval1.9+
Attached patch v4 (obsolete) — Splinter Review
In the previous version I forgot to add the js_ prefix to the AllocRawStack function.
Attachment #307131 - Attachment is obsolete: true
Attachment #307134 - Flags: review+
Attachment #307134 - Flags: approval1.9+
Attached patch v4bSplinter Review
This is a cvs diff version of the previous patch for references.
Attachment #307134 - Attachment is obsolete: true
Attachment #307219 - Flags: review+
Attachment #307219 - Flags: approval1.9+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 420902
Blocks: 420904
Blocks: 420906
Depends on: 421072
It seems this caused bug 419969. (Twinopen regression)
Of course I meant bug 421072 :)
fyi, just blocking1.9+'d Bug 421072 (Twinopen regression) as we need to look into it.
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.