Closed Bug 419969 Opened 12 years ago Closed 12 years ago
_Interpret or splitting jsinterp .c
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.
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.
Apparently using -falign-functions=4096 -O3 just for js_Interpret speedups the benchmark by 7%.
Now this quilt patch includes all files.
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.
New patch coming based on IRC discussion (jsinvoke.c and jsinterp.c)? /be
The new patch makes sure that js/scr compiles when JS_OPMETER is defined.
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
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
(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.
The new version syncs the patch with trunk and adds the guarding comment: #endif /* !defined js_invoke_c__ */
In the previous version I forgot to add the js_ prefix to the AllocRawStack function.
This is a cvs diff version of the previous patch for references.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.