Closed Bug 433337 Opened 12 years ago Closed 12 years ago

Reunify jsinterp.c on Windows

Categories

(Core :: JavaScript Engine, defect, P1)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: sayrer, Assigned: igor)

Details

(Keywords: fixed1.9.1, perf)

Attachments

(1 file, 2 obsolete files)

Splitting it caused a slight performance regression on windows. The file should be reunified there, and member functions should be declared static rather than extern.
Keywords: perf
Version: unspecified → Trunk
Adding this to the 1.9.1 triage queue.
Flags: blocking1.9.1?
Priority: -- → P1
Flags: blocking1.9.1? → blocking1.9.1+
OS: Linux → Windows XP
Attached patch v1 (obsolete) — Splinter Review
The patch makes helper functions for js_Interpret either static and compiled as a part of jsinterp.cpp or external depending on the value of the JS_LONE_INTERPRET macro.

By default JS_LONE_INTERPRET is set to 0 (static case) with MSVC and to otherwise.
Attachment #327773 - Flags: review?(brendan)
Comment on attachment 327773 [details] [diff] [review]
v1

>-#ifdef js_invoke_c__

Could we rename this jsinvoke_cpp___ to match the convention for header file idempotent-include guard macros? No extra underscores (js_) and the suffix is .cpp now, plus the triple _ at the end, IOW.

>+#if JS_LONE_INTERPRET
>+# define JS_INTERPRET_HELPER(declaration)       extern declaration;
>+# define JS_INTERPRET_HELPER_MODIFIER
>+#else
>+# define JS_INTERPRET_HELPER(declaration)
>+# define JS_INTERPRET_HELPER_MODIFIER           static
>+#endif
> 
>-extern JSClass *
>-js_IsActiveWithOrBlock(JSContext *cx, JSObject *obj, int stackDepth);
>+JS_INTERPRET_HELPER(jsval *
>+js_AllocRawStack(JSContext *cx, uintN nslots, void **markp))

These are overparenthesized and unsightly, and possibly confusing to ctags-ish tools. Would it be simpler and shorter to just #ifdef the .h file externs?

/be
Attached patch v2 (obsolete) — Splinter Review
The new version of the patch addresses the suggestions from the comment 3. In particular, now jsinterp.h simply ifdefs the declarations of the interpreter helpers.
Attachment #327773 - Attachment is obsolete: true
Attachment #328200 - Flags: review?(brendan)
Attachment #327773 - Flags: review?(brendan)
Attachment #328200 - Attachment is patch: true
Attachment #328200 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 328200 [details] [diff] [review]
v2

>+/*
>+ * JS_LONE_INTERPRET indicates that the compiler should see just the code for
>+ * the js_Interpret function when compiling jsinterp.cpp. The rest of the code
>+ * from the file should be visible only when compiling jsinvoke.cpp. It allows
>+ * to optimize selectively js_Interpret when the granularity of the

"It allows platform builds to optimize ...."

Looks good otherwise, thanks.

/be
Attachment #328200 - Flags: review?(brendan) → review+
Attached patch v2bSplinter Review
The new version of the patch fixes the comments:

  * from the file should be visible only when compiling jsinvoke.cpp. It allows
- * to optimize selectively js_Interpret when the granularity of the
- * optimizations with the given compiler is a compilation unit.
+ * platform builds to optimize selectively js_Interpret when the granularity
+ * of the optimizations with the given compiler is a compilation unit.
  *
Attachment #328200 - Attachment is obsolete: true
Attachment #329206 - Flags: review+
fixed: http://hg.mozilla.org/mozilla-central/index.cgi/rev/0f820c56c2f4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.