433337 - Reunify jsinterp.c on Windows

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Robert Sayre]

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.

Comment 1

[Damon Sicore (:damons)]

Adding this to the 1.9.1 triage queue.

Comment 2

[Igor Bukanov]

Attachment: v1

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.

Comment 3

[Brendan Eich [: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

Comment 4

[Igor Bukanov]

Attachment: v2

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.

Comment 5

[Brendan Eich [:brendan]]

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

Comment 6

[Igor Bukanov]

Attachment: v2b

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.
  *

Comment 7

[Igor Bukanov]

fixed: http://hg.mozilla.org/mozilla-central/index.cgi/rev/0f820c56c2f4