Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

No Ticket: jQuery in strict mode #998

Merged
merged 2 commits into from

9 participants

Julian Aubourg Mike Sherov Paul Irish John-David Dalton Rick Waldron Dannii Willis Scott González Timmy Willison Dave Methvin
Julian Aubourg
Collaborator

I wanted to see if anyone knew of any place in the code where we assume this to be window in functions called without a context before merging this in (that's the only change in behaviour I can think of but feel free to correct me, I'm not a strict mode specialist).

Also, I didn't enforce strict mode for the test directory yet, seeing as everything single test function would require a "use strict" declaration or we'd need to embed every unit file into an IIFE (something that could probably be done automagically but my eyes burn and I need to sleep right now).

Julian Aubourg jaubourg adds strict rule to jshint options (except for test files). "use stri…
…ct" is added to the main jQuery closure and some "could-be-unsafe" `this` trickery in effects is silenced.
9346c0e
Julian Aubourg
Collaborator

Also, the "use strict" directive has to stay in the minified file because it changes runtime behaviour.

.jshintrc
@@ -9,6 +9,7 @@
"node": true,
"quotmark": "double",
"smarttabs": true,
+ "strict": true,
Mike Sherov Collaborator

Unnecessary, as the node: true option covers this already.

Julian Aubourg Collaborator

Good to know. On the move today, feel free to hack in the branch ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Julian Aubourg jaubourg merged commit b386080 into from
Timmy Willison
Collaborator

hmm? I don't see how they're related. node is for allowing globals that are present in a node environment. strict is for checking strict mode in function scope.

Collaborator

Well, they should document that.

Collaborator

Yes, they certainly should!

Collaborator

Yes, I'm really not a fan of how jhint handles its flags. Too many implicit dependencies for its own good.

Collaborator

I wonder how much of a performance hit we'll be taking for this... strict mode is has significant performance costs that are attributed to the runtime checks that are required.

JFTR...

-1

Collaborator

I agree with @rwldrn on this one. Strict mode is cool and all, but represents a change that we should probably discuss together first before landing. If we did, and I missed that boat, I apologize. If not, we should back this out and talk about it first before re-landing.

Owner

They DID document it, right here! :trollface:

But in this case jshint is just checking against the static use strict requirements, it's not enforcing any runtime checks, right? I like the idea of being use strict clean but not sure I'm ready for a runtime relationship.

Collaborator

@rwldrn I thought a major point of strict mode was to help interpreters optimize. Is this a situation where it turned out to be a wash?

Collaborator

That's a side note. I'm +1 on removing for now.

Collaborator

So, what should we remove? jshint checking for strictness or "use strict" (as of now, you cannot have the first without the latest). I'm also as suprised as @timmywil regarding performance impact, I too thought the point was to make it easier to optimize code...

Collaborator

How do I go about benchmarking the strict build to see what the impact actually is? It feel wrong to me to take decisions without metrics. Never used our benchmarking suite so I'll need some help here.

Collaborator

It feel wrong to me to take decisions without metrics.

Exactly. The decision to add it needed to have metrics first :smiley:

Collaborator

It's not like anyone raised those concerns in the PR :P

Collaborator

Yeah, I missed it. To be honest, I think we were all a bit loopy by the third day!

Collaborator

I hear you ;)

Anyway, here is my line of reasoning: jQuery in strict mode is good (at least that's what I think). Concerns were raised about performance impact, let's actually establish this performance loss exists before removing. Because everything I read on the interweb (@rwldrn's comment got me really curious) seems to indicate there is strictly no perf impact one way or the other.

Collaborator

I'm mobile, so I can't really dig in and do all of the google-ing for everyone, but think of it like this: if process A has 2 steps and process B is the same algorithm, but takes 4 steps because it has additional runtime checks to evaluate (everytime), which will perform better? Obviously A, because it only takes 2 steps to complete.

Collaborator

@rwldrn: your theory is good, but I have a feeling it's too simplistic. Is strict mode for javascript engine optimization? Are you saying it helps compile time but not runtime? If so, aren't there more factors to consider during runtime than simply the number of steps (such as inline caching)?. Yes, under the worst of circumstances, a system designed to take 4 steps should take longer than 2 steps, but only if those 2 steps are identical or faster than the corresponding 2 of the 4 steps AND only if those 4 steps can never take shortcuts (perhaps turning 4 steps into 1). Even if strict compliance only helps the interpreter and not execution, there is still the question, "can the additional optimizations of the interpreter outweigh the extra steps taken during runtime?"

As @jaubourg pointed out, this all seems somewhat moot at these low levels anyway since browsers supporting "use strict" are fast enough that the difference is probably negligible.

Collaborator

@rwldrn: your theory is good, but I have a feeling it's too simplistic. Is strict mode for javascript engine optimization?

No, it's for turning common mistakes into thrown exceptions and locking out access to language features deemed "bad".

Are you saying it helps compile time but not runtime?

Some things are compile time errors and others are runtime.

If so, aren't there more factors to consider during runtime than simply the number of steps (such as inline caching)?.

Sure, but "strict mode" as it's specified, adds more work to the runtime.

Yes, under the worst of circumstances, a system designed to take 4 steps should take longer than 2 steps, but only if those 2 steps are identical or faster than the corresponding 2 of the 4 steps AND only if those 4 steps can never take shortcuts (perhaps turning 4 steps into 1). Even if strict compliance only helps the interpreter and not execution, there is still the question, "can the additional optimizations of the interpreter outweigh the extra steps taken during runtime?"

A few months ago, I wrote a complete use-case analysis of strictmode to help myself understand it, I've made a gist to share: https://gist.github.com/3920242

As @jaubourg pointed out, this all seems somewhat moot at these low levels anyway since browsers supporting "use strict" are fast enough that the difference is probably negligible.

Here's two v8 implementors: https://groups.google.com/forum/?fromgroups=#!topic/v8-users/QVXyJKNShJQ

Collaborator

No, it's for turning common mistakes into thrown exceptions and locking out access to language features deemed "bad".
Some things are compile time errors and others are runtime.

Let me rephrase. Leaving out runtime errors (which would seem to at least add extra conditionals in certain places), any compile time errors that may be determined before execution would theoretically help optimize execution, correct? For instance (and I'm just guessing here), when a variable must be found at runtime, we know it can be found before getting to the global object (unless it is explicitly a variable on the global object) if and only if we are in strict mode, since we already determined that there were no accidental globals at compile time.

Collaborator

Here's two v8 implementors: https://groups.google.com/forum/?fromgroups=#!topic/v8-users/QVXyJKNShJQ

No performance benefit !== performance hit.

No, it's for turning common mistakes into thrown exceptions and locking out access to language features deemed "bad".

I assumed that some of those "bad" things were deemed bad because of their performance and optimization implications, no?

The bottom line comes down to this: show me any data proving or suggesting that strict mode is slower and I'll immediately back down on that point. In the meantime, it seems like strict mode may be useful in exposing bugs that may otherwise be hidden. On the other hand, JSHint takes care of most of this stuff for us anyway.

Collaborator

The problem I'm having is that I've been "chiming in" whenever I have a moment as I travel back to Boston, and haven't been able to sit down and produce any worthwhile evidence. Anyway, I'm beginning to not give a shit about it.

Collaborator

Also... I'm pretty sure the code is already "strict mode" compliant, without adding the directive.

@mikesherov...

I assumed that some of those "bad" things were deemed bad because of their performance and optimization implications, no?

"No" indeed. You missed something: semantically bad. Most of the features that are blocked are semantic mistakes (of the language), blocked to protect the user code from doing insane things.

Collaborator

@mikesherov

Hmm, anyway, I'm not sure strict mode will like our globalEval function: http://whereswalden.com/2011/01/10/new-es5-strict-mode-support-new-vars-created-by-strict-mode-eval-code-are-local-to-that-code-only/

The strict mode propagation into eval may be a problem. AFAIK, the only reason we switched to eval rather than script tag injection is that firefox sometimes didn't execute injected script tag with inline code synchronously (it would block execution when other injected, non-async, script tag were pending -- think outbound JSONP). I dunno what the status on this issue is now but we may be able to revert to script tag injection if needs be (would make @jdalton happy actually).

@rwldrn

I know how it is to be on a mobile env ;) Any data you can come up with regarding a potential performance regression is welcome whenever you have time.

Also... I'm pretty sure the code is already "strict mode" compliant, without adding the directive.

Actually there was only one place where we were potentially breaking (but it's just that jhint's static analysis can go so far). However, testing for the code to be strict mode at build time but not being strict at runtime seems to completely defeat the purpose. That's probably why jshint insists on "use strict" being in files it tests for "strictness".

Also, I clearly remember that strict mode was sold on the promise that getting rid of the semantics issues ES4 had would help optimize code at runtime. Maybe it changed along the way (and I'd understand why) but it clearly wasn't just about static analysis (dunno why the last two words got eaten by the intertubes).

Collaborator

ES3* (no ES4, sorry to be pedantic). Ive never seen strict mode sold as an "optimization" (mostly because that's simply not what it's for)

Collaborator

@rwldrn Don't worry about being pedantic, we need pedantic people ;)

As for being sold as an optimization:
http://hacks.mozilla.org/2011/01/ecmascript-5-strict-mode-in-firefox-4/

What does strict mode do? First, it eliminates some JavaScript pitfalls that didn’t cause errors by changing them to produce errors. Second, it fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that’s not strict mode. Firefox 4 generally hasn’t optimized strict mode yet, but subsequent versions will. Third, it prohibits some syntax likely to be defined in future versions of ECMAScript.

Cannot be any clearer than that and it's not the only place where I saw such claims... but it was in early 2011 and I could understand if this didn't go as planned.

Collaborator

That source is not reputable, as they have published numerous erroneous statements and frequently allow code examples to become out dates. Furthermore, Chris Heilman is not an authority on the subject matter, nor does he back his claims with even the slightest shred of supporting content... But when I produce a point-by-point gist of all strict mode scenarios, I'm probably full of shit right?

Anyone can write a blog and submit it to hacks.mozilla...

Collaborator

Well, the MDN seems to agree with him:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Functions_and_function_scope/Strict_mode

Strict mode makes several changes to normal JavaScript semantics. First, strict mode eliminates some JavaScript pitfalls that didn't cause errors by changing them to produce errors. Second, strict mode fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that's not strict mode (Firefox 4 generally hasn't optimized strict mode yet, but subsequent versions will). Third, strict mode prohibits some syntax likely to be defined in future versions of ECMAScript.

The similar wording immediately caught my eye. Apparently (looking at the history of the page), a lot of infos from Chris Heilman's blog post has been ported pretty much verbatim into the MDN article, so maybe it's just the MDN people not being properly informed. If those claims are pure fantasy, then you have to submit edits to the MDN.

As for your gist, as informative and documented as it is, it just lists strict mode restrictions: it gives no indication of potential performance changes one way or the other. Performance impact is probably much more indirect than you seem to believe.

Let's look at the arguments object for instance: not having to expose the call stack through arguments.callee means that the VM doesn't have to adapt its internal call stack structure around it, giving opportunities for optimization. On the other hand, I'd expect enforcing the object is read-only to degrade performances. However, it cannot be worse that the overhead of trying to change the value of a field of a locked object (the former probably being implemented using the latter).

in the end, whatever our guess work, the impact of performance is impossible to evaluate mainly because most of the added logic is in the VM internals, so it's all dependent on internal architecture, native code optimization and native-to-JS bridging. Like I said, very complex. So, I still think we need to benchmark here.

However, from what I could gather in the last few hours, I doubt we'll see any significant performance degradation.

Collaborator

My gist only lists strict mode restrictions because that's all strict mode is... I've been trying to explain this. It's EXTRA instructions that the runtime must carry out.

Look, all I wanted was for everyone to think about the consequences first, before just jumping into something because JSHint says so. I got what I wanted and you all made your decision, so great, all in a days work.

As for MDN, it's a wiki, anyone can write anything on it and I know that Christian works on docs, so he likely copied it himself (that is purely speculation).

FWIW, I requested the ability to test for strict mode compatibility without actually using strict mode: jshint/jshint#504

It was apparently closed 2 months ago with no explanation :-(

https://groups.google.com/forum/?fromgroups=#!topic/v8-users/QVXyJKNShJQ might be relevant, though I don't know if jQuery ever does that.

http://stackoverflow.com/questions/3145966/is-strict-mode-more-performant/6103493#6103493 suggests that these additional checks will be made regardless of whether or not you use strict mode, but the implementations might do it differently.

Collaborator

That post was doing well until it invalidated itself by claiming that strict mode is "parse time"

Ok, just to my two cents. I used to have Lo-Dash default to having use strict in its minified code (before making it a build option for Underscore compat). I didn't really see a big negative perf impact from it.

Current engines do have some penalties for strict mode checks, but it's probably micro.
That said, I want to encourage use strict. I wouldn't be surprised to see engines start enticing use by optimizing for strict mode.

As for the global eval. jQuery's current use window[ "eval" ].call( window, data ) is an indirect eval call so considered global under ES5.

@paulirish Do you know if V8 is optimizing for strict mode atm? I have an inference in Lo-Dash for it because it seems to (if not, maybe it just handles strict mode code less worse than others).

Some use strict gotchas I ran into with Lo-Dash for Underscore compat were things like in non-strict mode, writing to a read-only property would not throw an error (it does in strict mode).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 18, 2012
  1. Julian Aubourg

    adds strict rule to jshint options (except for test files). "use stri…

    jaubourg authored
    …ct" is added to the main jQuery closure and some "could-be-unsafe" `this` trickery in effects is silenced.
Commits on Oct 19, 2012
  1. Julian Aubourg
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 0 deletions.
  1. +2 −0  src/.jshintrc
  2. +1 −0  src/effects.js
  3. +1 −0  src/intro.js
2  src/.jshintrc
View
@@ -6,8 +6,10 @@
"evil": true,
"expr": true,
"maxerr": 100,
+ "newcap": false,
"quotmark": "double",
"smarttabs": true,
+ "strict": true,
"sub": true,
"trailing": true,
"undef": true,
1  src/effects.js
View
@@ -233,6 +233,7 @@ jQuery.Animation = jQuery.extend( Animation, {
});
function defaultPrefilter( elem, props, opts ) {
+ /*jshint validthis:true */
var index, prop, value, length, dataShow, tween, hooks, oldfire,
anim = this,
style = elem.style,
1  src/intro.js
View
@@ -12,3 +12,4 @@
* Date: @DATE
*/
(function( window, undefined ) {
+"use strict";
Something went wrong with that request. Please try again.