Closed Bug 540675 Opened 10 years ago Closed 9 years ago

remove callerVersion from JIT stack frame

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: luke, Assigned: cdleary)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 2 obsolete files)

In the pursuance of slimmer stack frames, I was looking at killing  JSInlineStackFrame::callerVersion.  It seems that script versionining is hardly used at the moment.  Other than a lot of logic for copying and adjusting script versions (some on hot paths like JSOP_CALL), it seems that the only side effect is the toggling of JSOPTION_ANONFUNFIX.  JSOPTION_XML is also toggled, but that doesn't seem to be used anywhere.

So, (1) is there any other purpose served by cx->version/script->version I'm missing, and, if not, (2) can script->version go (or perhaps versions in general)?
JSOPTION_XML is certainly used:

jsscan.cpp:            (JS_HAS_XML_OPTION(cx) || PeekChar(ts) != '!')) {

Note that version and options bits for XML and ANONFUNFIX are sync'ed.

Script versioning is definitely used in order to enable let and yield as reserved words. Besides js/src/tests, you may find evidence of this in XUL apps and even some web pages.

For the future, we will need opt-in versioning based on RFC 4329 in the next major ECMA-262 edition, so I wouldn't pitch versioning support without a better replacement.

However, it would be good to simplify versioning. Right now version setting is "sticky" in order that a test can call version(180) and the jsinterp.cpp code just before returning will not restore originalVersion.

Nothing on the web or any add-on/app depends on this, since there is no version() builtin outside of the shells. This is probably the first target to attack.

/be
(In reply to comment #1)
Oops, yeah, didn't grep hard enough.

So, is it the case that cx->version is usually == fp->script->version and that custom frame versions (which actually require JSInlineStackFrame::callerVersion) are rare/only occur in js/shell tests?
(In reply to comment #2)
> (In reply to comment #1)
> Oops, yeah, didn't grep hard enough.
> 
> So, is it the case that cx->version is usually == fp->script->version and that
> custom frame versions (which actually require
> JSInlineStackFrame::callerVersion) are rare/only occur in js/shell tests?

Yes, only-occur-in-js/shell-tests. Whew!

/be
Ah, neato.  Then perhaps callerVersion can indeed be kicked out of the Jaeger/Trace-stack frame and attempting to set the current version to something other than cx->fp->script->version knocks you back to the interpreter.
Summary: can script versions go? → remove callerVersion from JIT stack frame
Blocks: 536275
Blocks: 557378
Grabbing this one.
Assignee: general → cdleary
Status: NEW → ASSIGNED
@shaver, lw says you had a notion of the JS_SetVersion behavior we want to preserve here?
it's my fervent hope that cross-version behaviour differences (rather than merely needing opt-in for new syntax) can be represented as differently-generated code, and therefore not need to be version-checked at runtime even when calling between functions of different versions.  eval is the obvious exception, and it can consult cx->fp->script->version.  Even then, it would be super-cool to not have runtime script version either, so when we emitted a call to eval we'd need to bake in the version information as additional call data.

I think JS_SetVersion should set cx->version only, and that should propagate to the version member of new scripts.  It's sort of a crappy pigeonhole, but I think it'll do?  I'm not *exactly* an expert on this, and generally think that we should be avoiding runtime version switching as much as possible.  I'd *love* to just get rid of SetVersion and add the 17th parameter to JS_CompileScript's family to specify the version there.

I don't know if that was helpful at all, sorry.
Attached patch WIP: remove caller version. (obsolete) — Splinter Review
WIP: needs comments.


Background
----------

The version-switching algorithm has the following properties that are actually observed by the tests:

- If you set the version and revert it before you ever call an interpreted function, the interpreter pretends nothing ever happened. This lets you use a set-version/compile/unset-version idiom -- you just have to be sure you're only calling natives.
- If you set the version and then invoke an interpreted function, the version that you set will "stick" until the current js::Interpret invocation (inline calls and all) completes. You can't obey this property without doing some kind of update in the interpreted function call path, so we trash this.
- If the version isn't stuck, when scripts are invoked, their version gets pushed into the context.

This WIP patch simplifies things:

- You have a "default" compilation version, which gets locked in when you call JS_SetVersion and there is no code executing. (This is commonly done by embedders after a new context is created.)
- The "default" compilation version is superseded by the version of the nearest script executing on the stack, if there is one.
- The nearest script executing on the stack can be "overridden". When you call JS_SetVersion and there is code executing, it is understood as an override.
- The override is reverted by passing a special version value to JS_SetVersion, JSVERSION_UNDO. This ties into the new |unversion| function in the shell.

Followup
--------

We should remove JS_SetVersion and JS_GetVersion entirely -- I took a look, and all browser invocations can be transformed into an extra Compile/Evaluate/* interface that takes a version parameter.
Followup bugs to file if this patch looks okay:
- Switch browser away from JS_Set/GetVersion API and remove it, making JSVERSION_MASK and friends non-visible from jscntxt.h.
- Move js::Version into a mutually accessible place from jsscript.h and jscntxt.h and switch script->version to the new, slightly safer, interface.
Attachment #471590 - Attachment is obsolete: true
Attachment #471692 - Flags: review?(lw)
Comment on attachment 471692 [details] [diff] [review]
Remove JSStackFrame::callerVersion.

This looks good; its nice to see all that version code removed!

A few general comments:

I think you can remove JSVERSION_UNDO and just make a JS_FRIEND_API function in jsdbgapi that gets called by shell/js.cpp.  Also, I would rename the shell function to perhaps revertVersion().  Unversion is a bit confusing.

I don't really see any benefit of sticking the JSVERSION_* constants in namespace js.  They already have JS in the name, so unless you want to totally rename them all to this, it just seems like noise:

 namespace js {
   namespace VersionFlags {
     enum Type {
       HasXML = 0x1000, 
       // etc
     };
   }
 }

Lastly, I don't really see the need for the new js::Version class.  It mostly just seems to add utility functions for manipulating JSVersion values.  Perhaps you can just make a set of non-member functions that achieve the same effect?  That would reduce a lot of the js::Version <--> JSVersion noise.  I feel that, to actually simplify things, we would need to make more fundamental changes (as you mentioned in the above comments) than adding a new abstraction like js::Version.
Definitely nice to avoid those gross comparison functions (sameNumberAs, sameExactAs, etc.), but it's easier for people to slip up because they're not forced to think about the comparison they want to perform. All in all, a group of functions certainly made it prettier!
Attachment #471692 - Attachment is obsolete: true
Attachment #472546 - Flags: review?(lw)
Attachment #471692 - Flags: review?(lw)
Comment on attachment 472546 [details] [diff] [review]
Remove JSStackFrame::callerVersion.

Thanks, looks good.  r+ with a couple small requests:

I don't think its worth doing it now, but I was thinking that cx->findVersion could cache its result, which would decrease the potential for slowdown in pathological scenarios.  But perhaps you could put a comment in findVersion that says "if this ever shows up in a profile, just add caching"?

>+static inline void
>+VersionEnableXML(JSVersion *version, bool enable)

Since this function is only conditionally enabling XML, perhaps it could be named VersionSetXMLOption or something?

>+static inline void
>+VersionEnableAnonFunFix(JSVersion *version, bool enable)

Same

>+  private:
>+    /* See JSContext::findVersion. */
>+    JSVersion           defaultVersion;      /* script compilation version */
>+    JSVersion           versionOverride;     /* supercedes defaultVersion when valid */
>+    bool                versionOverrideValid;

Perhaps, for continuity with other code, 'bool hasVersionOverride;' ?

>         EVAL_CACHE_METER(probe);
>         while ((script = *scriptp) != NULL) {
>             if (script->savedCallerFun &&
>                 script->staticLevel == staticLevel &&
>-                script->version == cx->version &&
>+                script->getVersion() == cx->findVersion() &&
>                 (script->principals == principals ||
>                  (principals->subsume(principals, script->principals) &&
>                   script->principals->subsume(script->principals, principals)))) {

Let's hoist that O(n) algorithm out of the while loop.

> JS_FRIEND_API(void)
>+js_DumpOptions(JSContext *cx)

It seems like this function could be written more readably as just a chain of:

 if (options & JSOPTION_STRICT) fprintf(stderr, "|STRICT");
 if (options & JSOPTION_WERROR) fprintf(stderr, "|WERROR");
 ...
 if (options) fprintf(stderr, "|");

>@@ -506,16 +509,17 @@ class TokenStream
>     bool                maybeEOL[256];  /* probabilistic EOL lookup table */
>     bool                maybeStrSpecial[256];/* speeds up string scanning */
>+    JSVersion             version;      /* cached version number for scan */

align 'version'

> static JSBool
>+RevertVersion(JSContext *cx, uintN argc, jsval *vp)
>+{
>+    js_RevertVersion(cx);
>+    *vp = JSVAL_VOID;
>+    return JS_TRUE;
>+}

For purity, JS_SET_RVAL(cx, vp, JSVAL_VOID)
Attachment #472546 - Flags: review?(lw) → review+
Blocks: 539144
(In reply to comment #12)

Just a few small questions.

> >+static inline void
> >+VersionEnableXML(JSVersion *version, bool enable)
> 
> Since this function is only conditionally enabling XML, perhaps it could be
> named VersionSetXMLOption or something?

Since version flags get sync'd from cx->options, I'd prefer to leave "option" out of the name if you don't feel strongly about it.

> >+  private:
> >+    /* See JSContext::findVersion. */
> >+    JSVersion           defaultVersion;      /* script compilation version */
> >+    JSVersion           versionOverride;     /* supercedes defaultVersion when valid */
> >+    bool                versionOverrideValid;
> 
> Perhaps, for continuity with other code, 'bool hasVersionOverride;' ?

I thought a "has" prefix was similar to an "is" prefix in indicating a method-verby thing? I purposefully tried to noun it.

> Let's hoist that O(n) algorithm out of the while loop.

Awesome catch. This is why we have code reviews. :-D

> > JS_FRIEND_API(void)
> >+js_DumpOptions(JSContext *cx)
> 
> It seems like this function could be written more readably as just a chain of:
> 
>  if (options & JSOPTION_STRICT) fprintf(stderr, "|STRICT");
>  if (options & JSOPTION_WERROR) fprintf(stderr, "|WERROR");
>  ...
>  if (options) fprintf(stderr, "|");

But without the loop you can place an extra bar, no?
(In reply to comment #13)
> Since version flags get sync'd from cx->options, I'd prefer to leave "option"
> out of the name if you don't feel strongly about it.

Leaving "option" out is fine, it was just the "Enable" I took issue with.

> > Perhaps, for continuity with other code, 'bool hasVersionOverride;' ?
> 
> I thought a "has" prefix was similar to an "is" prefix in indicating a
> method-verby thing? I purposefully tried to noun it.

"has version override" is a descriptive phrase, so it doesn't seem too verb-y.

> > > JS_FRIEND_API(void)
> > >+js_DumpOptions(JSContext *cx)
> > 
> > It seems like this function could be written more readably as just a chain of:
> > 
> >  if (options & JSOPTION_STRICT) fprintf(stderr, "|STRICT");
> >  if (options & JSOPTION_WERROR) fprintf(stderr, "|WERROR");
> >  ...
> >  if (options) fprintf(stderr, "|");
> 
> But without the loop you can place an extra bar, no?

That's what the "if (options)" part was for.
Whiteboard: fixed-in-tracemonkey
Adds version-specific compilation APIs to solve the mochitest-chrome server problem that we were having (which caused the backout). Could have tried to replicate the old behavior (popping a version override when the Interpret loop exits), but this is heading the right direction anyway. File up bug can eliminate other invocations of JS_SetVersion in the browser, none of them look too difficult.
Attachment #474345 - Flags: review?(lw)
Comment on attachment 474345 [details] [diff] [review]
Add version-specifying compile interfaces.

Good job tracking down this weirdness.  I look forward to us fixing the more fundamental issues that are causing all this version/option/global-state ad-hoc-ery.  Is there a bug # for that yet?
Attachment #474345 - Flags: review?(lw) → review+
(In reply to comment #17)
> Is there a bug # for that yet?

Just filed as bug 595691.
Optimistic, marking as fixed-in-tracemonkey.
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/33bf77bcf1a0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.