[patch][cla] lang._mixin and has("bug-for-in-skips-shadowed")

[mm]

It is not effective to call function with each mixin. Specially if we talk about MSIE 6 probably. Included patch is proposed solution. Testcases passed: ​http://localhost:8080/ngs/samples/ui/resources/util/doh/runner.html?testModule=dojo.tests._base.lang

I have found intensive usage of mixin in the charting stack, where single animated grapt can call this thousands times. This patch could help

[bill]

Are you talking about performance?

You said

It is not effective to call function with each mixin.

but it's unclear what that means..

[mm]

yes performance ;-)

[Kenneth G. Franqueiro]

I'm attaching a patch formatted such that it should be able to be applied to the SVN checkout. Did some testing and it doesn't seem to regress so that's good.

HOWEVER, while this does seem to considerably improve performance in browsers which don't need the extra logic, it actually noticeably worsens performance in browsers which do need it (I tested IE6-7; not sure if 8 does as well, 9 doesn't). I'm talking a significant 33% increase in execution time.

IMO this consequence outweighs the benefit, since the non-problem browsers are significantly faster to begin with. Perhaps this would be a better idea for 2.0?

I'm attaching a standalone test page for reference (not using jslitmus to avoid IE's "script taking too long" warnings which will totally throw off the results).

(Also, FWIW, if you're only targeting modern browsers or the shadowing issue doesn't concern you, you can do a build optimizing with Closure and setting this has-feature to 0 in staticHasFeatures, and this whole issue should become moot.)

[Kenneth G. Franqueiro]

test page for regression-testing and getting a quick perf readout in old IE

[Kenneth G. Franqueiro]

diff formatted for svn

[mm]

Sorry can you supply some readining why the new version is 33% slower ? Correct test case woul be including old and new module and make perf test of both. I see no reason for this claim. In msie 6 both functions will be the same in runtime.

[Kenneth G. Franqueiro]

It's quite simple to run the attached test against both versions (which is what I did): take current trunk, run test, swap out lang.js with patched version, run test again. I'm only telling you what I saw myself doing the same thing, it's not like I'm making this up for some reason...

[Eugene Lazutkin]

Replying to mm:

I see no reason for this claim.

Your patch replaced if with a function call. The latter is more expensive operation than an if, or almost any other logical or arithmetical operation, especially on IE. (In fact if your function is a one-liner, it almost always makes sense to inline it rather than call it as a function.)

The current implementation was tested on all supported browsers, and found to be a good compromise --- it was "fast enough" on fast modern browsers, and "fast enough" on IE.

The faster implementation would be to create two separate versions of the function: one for modern browsers, and one for IE6, but we decided against it for space-saving reasons.

[Kenneth G. Franqueiro]

Well, to be clear, there's a function call within the if which is what he's trying to avoid, and the patch basically is going the two-separate-versions route. But yes, there is still the extra function call in the case of browsers that need the extra code (old IE).

[Kenneth G. Franqueiro]

Bleh, sorry, I just realized what you meant Eugene - you meant create two entirely separate functions, but yes, clearly that would come at a significant byte cost and would involve repeating code, which seems suboptimal.

[mm]

Maybe I;m counting something wrong but: a) original solution is n * (code1 + if + function call (has)+ code1), in both, new and old browsers, new browsers b) new solution is 1*(if+function call(has)) + n*(code1) in new browsers and 1*(if+function call(has)) + n * (code1+code2)

so please explain me where the number of function calls gets increased ? and how this can be slower ? (hard to believe, I'm really building realistic tests now)

And when talking about size, must (shall) be measured minified and compressed.

Also the number of clone, mixin and other calls goint this route shall be estimated based on more realistic scenarios (average RIA page with 10th of widgets, and 3-4 charts ?) Then we talking about significant number of calls and inclusive time saved.

[mm]

C:\dev\sts-workspace\NGS\azlk-ngs-samples-ui\src\main\webapp\WEB-INF\views\test\lang-mixin-test\test.js

[mm]

Please can you try to run the included test case ? It does not support any of your concerns. Sorry.

[Eugene Lazutkin]

I don't understand the problem. The code was very simple, and your proposal is very simple too. It is easy to count operations.

The current code:

function mixin(d, s){
  // loop
  if(...){
    // extra loop (xloop)
  }
}

Your proposal, the modern browser case:

function mixin(d, s){
  // loop
}

As you can see there is no if, which is good.

Your proposal, the IE6 case:

function mixin(d, s){
  // CALL to a function above
  // extra loop (xloop)
}

As you can see:

Code	Modern browser	IE6
Current code	CALL+LOOP+IF	CALL+LOOP+IF+XLOOP
Your proposal	CALL+LOOP	CALL+CALL+LOOP+XLOOP

As you can see with your code the difference is as follows:

1. The modern browser is faster by one if.
2. IE6 is slower by the difference call-if.

Because if is relatively fast, while call is much more expensive than if, your code gains little on modern browsers, yet slows down quite a bit on IE6, which is as slow as they can be already without our help.

And your code doesn't have any gain byte-wise.

I hope it will be fair to close this enhancement at least for the 1.x line.

[mm]

One of us (I admit its me, being just tired) is reading this badly, sorry no to give up: current code is:

Modern: _MIXIN=LOOP+IF(CALL_HAS) there is call to has() in every call to mixin IE6: _MIXIN=LOOP+IF(CALL_HAS) + XLOOP

My Proposal is: Modern: _MIXIN=LOOP IE6: _MIXIN=CALL_MIXIN+XLOOP

CALL_HAS is performed only once.

Dont forget the has() is function call, now it is replaced with other function call to mixin().

Sorry for not beuaty formating effort.

[bill]

You are forgetting the call from _mixin() to __mixin().

[Eugene Lazutkin]

Replying to mm:

Dont forget the has() is function call, now it is replaced with other function call to mixin().

I stand corrected --- I missed that the original if that checked a variable was replaced with a heavy-weight call to has(). We should revert it, and call has() only once --- it doesn't make any sense to call it multiple times.

Something like that:

var bugForInSkipsShadowed = has("bug-for-in-skips-shadowed");
...
// later in mixin():
if(bugForInSkipsShadowed){
  ...
}

And we should get rid of totally unnecessary if(source).

[Eugene Lazutkin]

Hmm, removing has() has a potential to subvert a build optimization. Nevertheless I think in this case we can afford it. In general has() mechanism is not very suited for fast functions. The reasonable alternative is to provide totally different paths for IE6 and the rest, which will involve some code duplication (removed by the build optimization).

[mm]

looking at this now. I see replacing has() call to has variable, would be much easier than my original patch.

subvert a build optimization: I do not think so, it does not metter if the has() is inside _mixin function call, or inside module scope, in both cases it shall be able to be used by build. (ternary rewritten to if maybe ?) must test.

[Kenneth G. Franqueiro]

Replying to elazutkin:

Replying to mm:

Dont forget the has() is function call, now it is replaced with other function call to mixin().

I stand corrected --- I missed that the original if that checked a variable was replaced with a heavy-weight call to has(). We should revert it, and call has() only once --- it doesn't make any sense to call it multiple times.

Something like that:

var bugForInSkipsShadowed = has("bug-for-in-skips-shadowed");
...
// later in mixin():
if(bugForInSkipsShadowed){
  ...
}

This occurred to me earlier today as well; however, as you later noted, I'm not sure offhand whether the Closure compiler is smart enough to follow this trail and still optimize away the dead code in the case where this were set to 0 in staticHasFeatures.

And we should get rid of totally unnecessary if(source).

No, we shouldn't. If I understand correctly, it's not totally unnecessary, it provides resilience if someone happens to pass null or undefined as a subsequent argument to mixin. Let's not get ahead of ourselves and risk regressions this close to beta :)

[mm]

"And we should get rid of totally unnecessary if(source)." is not my idea and is not part of patch. Looking at my original patch it shall be clear. Only we have to see if build can remove that code paths (ternary if). I have never tried this.

We cannot remove the has("bug-for-in-skips-shadowed"), since it is used by declare.js, query.js and some profiles as well.

And BTW: here is the perf test result with static bugForInSkipsShadowed version if someone is interested. MSIE 9.

TEST: lang_false

TRIAL SIZE: 40960 iterations
NUMBER OF TRIALS: 50
AVERAGE TRIAL EXECUTION TIME: 161.2600000000ms.
MAXIMUM TEST ITERATION TIME: 0.0040771484ms.
MINIMUM TEST ITERATION TIME: 0.0038330078ms.
AVERAGE TEST ITERATION TIME: 0.0039370117ms.
MEDIAN TEST ITERATION TIME: 0.0039306641ms.
VARIANCE TEST ITERATION TIME: 0.0000000050ms.
STANDARD DEVIATION ON TEST ITERATION TIME: 0.0000708950ms.

TEST: lang2_false

TRIAL SIZE: 40960 iterations
NUMBER OF TRIALS: 50
AVERAGE TRIAL EXECUTION TIME: 114.4200000000ms.
MAXIMUM TEST ITERATION TIME: 0.0029052734ms.
MINIMUM TEST ITERATION TIME: 0.0027099609ms.
AVERAGE TEST ITERATION TIME: 0.0027934570ms.
MEDIAN TEST ITERATION TIME: 0.0027832031ms.
VARIANCE TEST ITERATION TIME: 0.0000000021ms.
STANDARD DEVIATION ON TEST ITERATION TIME: 0.0000458283ms.

TEST: lang_true

TRIAL SIZE: 40960 iterations
NUMBER OF TRIALS: 50
AVERAGE TRIAL EXECUTION TIME: 161.2400000000ms.
MAXIMUM TEST ITERATION TIME: 0.0040771484ms.
MINIMUM TEST ITERATION TIME: 0.0038330078ms.
AVERAGE TEST ITERATION TIME: 0.0039365234ms.
MEDIAN TEST ITERATION TIME: 0.0039306641ms.
VARIANCE TEST ITERATION TIME: 0.0000000043ms.
STANDARD DEVIATION ON TEST ITERATION TIME: 0.0000654297ms.

TEST: lang2_true

TRIAL SIZE: 40960 iterations
NUMBER OF TRIALS: 50
AVERAGE TRIAL EXECUTION TIME: 114.2800000000ms.
MAXIMUM TEST ITERATION TIME: 0.0029052734ms.
MINIMUM TEST ITERATION TIME: 0.0027343750ms.
AVERAGE TEST ITERATION TIME: 0.0027900391ms.
MEDIAN TEST ITERATION TIME: 0.0027832031ms.
VARIANCE TEST ITERATION TIME: 0.0000000019ms.
STANDARD DEVIATION ON TEST ITERATION TIME: 0.0000431349ms.

[Eugene Lazutkin]

Replying to kgf:

If I understand correctly, it's not totally unnecessary, it provides resilience if someone happens to pass null or undefined as a subsequent argument to mixin.

No it doesn't. It protects the IE6 "extra loop", but the main loop, which runs first, is unprotected, so this check will never be executed --- the blow up will happen before it hits this if. Just look at the code.

Again, I've noticed that "improvement" because it was not there before. I agree with you 100% that we should stay conservative at least in our core functions.

And why is the main code not protected? Because in the best Dojo traditions we support ​GIGO "error check" whenever possible: when it blows up you know exactly where and why. Anything else is just hiding a problem making it harder to diagnose.

PS: Another "new feature" is a support for an optional function (the 3rd argument) that can be applied to each and every copied property. And it is checked for every copied property! Why? Is it expected to be different from property to property? I don't think so. It is a major slow down for a primitive operation --- roughly doubling the operation count per property. In any case, if somebody wants to do that --- just write the cycle using ... (gasp!) JavaScript. It is a one-liner. Do not overload _mixin() with unnecessary bells and whistles. Ironically mixin() doesn't use this facility at all. Want to do the IE6-specific processing too? That's why _extraNames are exposed outside.

PPS: If somebody wants to do a custom processing for an assignment of every property, this is the wrong way to do it, because it is slow yet limited. Wrap the whole assignment like that:

// just a sketch
function complexMixin(dst, src, copyFn){
  for(var name in src){
    copyFn(dst, src, name);
  }
}

And keep it separate from _mixin() in a separate file, so it can be loaded when needed. BTW, observe that there are no unnecessary checks in this version.

This way copyFn() can be used to implement a variety of clever name-specific processing techniques and a complex weaving. For example, instead of copying it can attach all functions with names like "onXXX" as events/aspects to corresponding methods of the destination instead of simply clobbering the destination. And yes, you can implement copyFunc as well.

[mm]

Nice ideas nice chat. Any decisions ?

[Colin Snover]

1.8 has been tagged; moving all outstanding tickets to next major release milestone.

[dylan]

Given the lengthy discussions on this, I think we should decide on the solution, fix, and close it out...

[Eugene Lazutkin]

Replying to dylan:

Given the lengthy discussions on this, I think we should decide on the solution, fix, and close it out...

So, what did you decide?

[dylan]

Replying to elazutkin:

Replying to dylan:

Given the lengthy discussions on this, I think we should decide on the solution, fix, and close it out...

So, what did you decide?

I don't think it really benefits anyone for me to make the decision on this one. Again, we're at a deadline for a release without a clear decision on what to do with it nearly two years later. I would assume we want to decide and get on with it, since this is something that will live on with 1.x for a while longer.

At this point, I have no choice but to punt it until 1.11, and hope that someone cares enough to address it after 1.10 is out.

[dylan]

Given the lack of recent interest, closing this out. If someone wants to create a pull request and show the performance is faster, go for it. Given that we no longer officially support IE 6 and 7, it might be an easier thing to achieve now...