Opened 9 years ago
Closed 5 years ago
#15478 closed enhancement (patchwelcome)
[patch][cla] lang._mixin and has("bug-for-in-skips-shadowed")
Reported by: | mm | Owned by: | dylan |
---|---|---|---|
Priority: | high | Milestone: | 1.11 |
Component: | Core | Version: | 1.7.2 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
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
Attachments (4)
Change History (31)
Changed 9 years ago by
comment:1 Changed 9 years ago by
Component: | General → Core |
---|
comment:3 Changed 9 years ago by
Owner: | set to Eugene Lazutkin |
---|---|
Status: | new → assigned |
comment:4 Changed 9 years ago by
Milestone: | tbd → 1.8 |
---|---|
Priority: | undecided → high |
Summary: | lang._mixin and has("bug-for-in-skips-shadowed") → [patch][cla] lang._mixin and has("bug-for-in-skips-shadowed") |
comment:5 Changed 9 years ago by
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.)
Changed 9 years ago by
Attachment: | mixin-15478.html added |
---|
test page for regression-testing and getting a quick perf readout in old IE
comment:6 follow-up: 8 Changed 9 years ago by
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.
comment:7 Changed 9 years ago by
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...
comment:8 Changed 9 years ago by
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.
comment:9 Changed 9 years ago by
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).
comment:10 Changed 9 years ago by
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.
comment:11 Changed 9 years ago by
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.
Changed 9 years ago by
C:\dev\sts-workspace\NGS\azlk-ngs-samples-ui\src\main\webapp\WEB-INF\views\test\lang-mixin-test\test.js
comment:12 Changed 9 years ago by
Please can you try to run the included test case ? It does not support any of your concerns. Sorry.
comment:13 Changed 9 years ago by
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
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:
- The modern browser is faster by one
if
. - 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.
comment:14 follow-up: 16 Changed 9 years ago by
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.
comment:16 follow-up: 19 Changed 9 years ago by
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
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)
.
comment:17 Changed 9 years ago by
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).
comment:18 Changed 9 years ago by
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.
comment:19 follow-up: 21 Changed 9 years ago by
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 tohas()
. We should revert it, and callhas()
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 :)
comment:20 Changed 9 years ago by
"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.
comment:21 Changed 9 years ago by
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.
comment:23 Changed 9 years ago by
Milestone: | 1.8 → 2.0 |
---|
1.8 has been tagged; moving all outstanding tickets to next major release milestone.
comment:24 follow-up: 25 Changed 8 years ago by
Milestone: | 2.0 → 1.10 |
---|
Given the lengthy discussions on this, I think we should decide on the solution, fix, and close it out...
comment:25 follow-up: 26 Changed 7 years ago by
Owner: | changed from Eugene Lazutkin to dylan |
---|---|
Status: | reopened → assigned |
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?
comment:26 Changed 7 years ago by
Milestone: | 1.10 → 1.11 |
---|
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.
comment:27 Changed 5 years ago by
Resolution: | → patchwelcome |
---|---|
Status: | assigned → closed |
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...
Are you talking about performance?
You said
but it's unclear what that means..