Fix behavior regression, added tests and supporting API #155

Merged
merged 28 commits into from Jun 20, 2012

Conversation

Projects
None yet
2 participants
@kberg
Collaborator

kberg commented Jun 15, 2012

There's quite a lot in here. 10 new automated tests, and some new infrastructure (an Iterator to make some code easier to understand, and consolidating the draw{Non,}TrivialLine methods using a strategy based on whether lines are drawn with stroke style.)

I can't tell how good the code is - your opinion is more than welcome.

Also, I haven't been able to do any performance testing - I'll do that tonight or tomorrow, but I will be surprised if timings go up significantly.

Robert Konigsberg added some commits Jun 13, 2012

Robert Konigsberg
connectSeparatedPoints is working. This has also improved
tests/independent-series.html, which was broken with the performance
enhancement, but that's still broken.
Robert Konigsberg
Introduce Dygraph.createIterator, and use it to simplify the code in …
…drawTrivialLine and drawNonTrivialLine. Lots o' tests.
Robert Konigsberg
Add some power to the canvas assertions. Also, the canvas assertions …
…weren't actually testing attributes. Fortunately they all passed.
Robert Konigsberg
Recombine drawTrivialLine and drawNonTrivialLine, using a strategy to…
… separate out the behavior. I haven't had a chance to performance test this yet. We shall see.
@kberg

This comment has been minimized.

Show comment
Hide comment
@kberg

kberg Jun 16, 2012

Collaborator

There is a performance hit, about 25%, it turns out. I'll see what may be causing it and try to optimize it out. One possibility is the extra moveTos, and another is the extra indirection. I'd still suggest pulling in this request, because it is more correct.

Collaborator

kberg commented Jun 16, 2012

There is a performance hit, about 25%, it turns out. I'll see what may be causing it and try to optimize it out. One possibility is the extra moveTos, and another is the extra indirection. I'd still suggest pulling in this request, because it is more correct.

@kberg

This comment has been minimized.

Show comment
Hide comment
@kberg

kberg Jun 16, 2012

Collaborator

Well, according to my tests using the benchmark (tests/dygraph-many-points-benchmark.html?points=1000&series=100&repetitions=100) the bump is due primarily to the iterator change and the final one. I'd hate to unroll the iterator, but if needs must.

Collaborator

kberg commented Jun 16, 2012

Well, according to my tests using the benchmark (tests/dygraph-many-points-benchmark.html?points=1000&series=100&repetitions=100) the bump is due primarily to the iterator change and the final one. I'd hate to unroll the iterator, but if needs must.

@kberg

This comment has been minimized.

Show comment
Hide comment
@kberg

kberg Jun 16, 2012

Collaborator

I can't be positive, but I think these optimizations have saved about 10% CPU time (after going up about 25%.)

Collaborator

kberg commented Jun 16, 2012

I can't be positive, but I think these optimizations have saved about 10% CPU time (after going up about 25%.)

@danvk

This comment has been minimized.

Show comment
Hide comment
@danvk

danvk Jun 18, 2012

Collaborator

I believe it's better to define new classes in the same way we do in the rest of the dygraphs code, i.e.

Dygraph.Iterator = function(...) {
this.idx_ = ...
this.end_ = ...
};

Dygraph.Iterator.prototype.hasNext = function() {
return this.nextIdx_ < this.end_;
};

...

And then

Dygraph.createIterator = function(array, start, length, predicate) {
return new Dygraph.Iterator(array, start, length, predicate);
};

This way we won't have to redefine the Iterator class every time an iterator is created.

Collaborator

danvk commented on dygraph-utils.js in 7d1afbb Jun 18, 2012

I believe it's better to define new classes in the same way we do in the rest of the dygraphs code, i.e.

Dygraph.Iterator = function(...) {
this.idx_ = ...
this.end_ = ...
};

Dygraph.Iterator.prototype.hasNext = function() {
return this.nextIdx_ < this.end_;
};

...

And then

Dygraph.createIterator = function(array, start, length, predicate) {
return new Dygraph.Iterator(array, start, length, predicate);
};

This way we won't have to redefine the Iterator class every time an iterator is created.

@danvk

This comment has been minimized.

Show comment
Hide comment
@danvk

danvk Jun 18, 2012

Collaborator

vlues -> vlaues

Should be pretty easy to do:

start = start || 0;
length = length || array.length;

Collaborator

danvk commented on dygraph-utils.js in 7d1afbb Jun 18, 2012

vlues -> vlaues

Should be pretty easy to do:

start = start || 0;
length = length || array.length;

This comment has been minimized.

Show comment
Hide comment
@kberg

kberg Jun 18, 2012

Owner

I'm pretty sure you meant values, and not vlaues. :)

fixed.

Owner

kberg replied Jun 18, 2012

I'm pretty sure you meant values, and not vlaues. :)

fixed.

@danvk

This comment has been minimized.

Show comment
Hide comment
@danvk

danvk Jun 18, 2012

Collaborator

There is actually an official notion of an "Iterator" in the JS language:
https://developer.mozilla.org/en/JavaScript/Guide/Iterators_and_Generators

I believe the only difference between it and your iterators is that they "throw StopIteration" after the final element, rather than returning null.

A nice win of using the standard iterators is that they can be used in "for (var k in iter) { ... }"-style loops.

Collaborator

danvk commented on dygraph-utils.js in 7d1afbb Jun 18, 2012

There is actually an official notion of an "Iterator" in the JS language:
https://developer.mozilla.org/en/JavaScript/Guide/Iterators_and_Generators

I believe the only difference between it and your iterators is that they "throw StopIteration" after the final element, rather than returning null.

A nice win of using the standard iterators is that they can be used in "for (var k in iter) { ... }"-style loops.

This comment has been minimized.

Show comment
Hide comment
@kberg

kberg Jun 18, 2012

Owner

Iterator is only available with 1.7.

var lang = { name: 'JavaScript', birthYear: 1995 };
var it = Iterator(lang);
ReferenceError: Iterator is not defined

Owner

kberg replied Jun 18, 2012

Iterator is only available with 1.7.

var lang = { name: 'JavaScript', birthYear: 1995 };
var it = Iterator(lang);
ReferenceError: Iterator is not defined

This comment has been minimized.

Show comment
Hide comment
@danvk

danvk Jun 18, 2012

Collaborator

Right, I was just suggesting that we use an API that's compatible with it.

Collaborator

danvk replied Jun 18, 2012

Right, I was just suggesting that we use an API that's compatible with it.

@kberg

This comment has been minimized.

Show comment
Hide comment
@kberg

kberg Jun 18, 2012

Owner

Good point, thanks. Done.

Owner

kberg commented on 7d1afbb Jun 18, 2012

Good point, thanks. Done.

@kberg

This comment has been minimized.

Show comment
Hide comment
@kberg

kberg Jun 18, 2012

Collaborator

(All comments responded to.)

Collaborator

kberg commented Jun 18, 2012

(All comments responded to.)

@danvk

This comment has been minimized.

Show comment
Hide comment
@danvk

danvk Jun 18, 2012

Collaborator

I realize that none of the other parameters here are documented, but we should at least document what "iter" is an iterator on, and possibly rename it (to something like "pointsIter").

Collaborator

danvk commented on dygraph-canvas.js in 7d1afbb Jun 18, 2012

I realize that none of the other parameters here are documented, but we should at least document what "iter" is an iterator on, and possibly rename it (to something like "pointsIter").

@danvk

This comment has been minimized.

Show comment
Hide comment
@danvk

danvk Jun 18, 2012

Owner

OK, are we good to pull? What does the net effect on performance wind up being? Is this a 10% win, or was the 10% win a win on the 25% loss (so only a 22.5% loss, or maybe a 15% loss)?

Owner

danvk commented Jun 18, 2012

OK, are we good to pull? What does the net effect on performance wind up being? Is this a 10% win, or was the 10% win a win on the 25% loss (so only a 22.5% loss, or maybe a 15% loss)?

@kberg

This comment has been minimized.

Show comment
Hide comment
@kberg

kberg Jun 18, 2012

Collaborator

Let me clarify: The post-optimization performance seemed to be about 'x', my first cut raised it to 1.25x, and the tweaks I made lowered it to 1.125x (in other words, 10% of 125 is 12.5)

Collaborator

kberg commented Jun 18, 2012

Let me clarify: The post-optimization performance seemed to be about 'x', my first cut raised it to 1.25x, and the tweaks I made lowered it to 1.125x (in other words, 10% of 125 is 12.5)

@kberg

This comment has been minimized.

Show comment
Hide comment
@kberg

kberg Jun 19, 2012

Collaborator

So, unrolling the iterator doesn't make an appreciable difference in performance (AFAICT) - at the moment I had to take that measurement on Linux/Chrome, because my Mac is acting really odd (aka SLOW.) I think this code is good to go.

Collaborator

kberg commented Jun 19, 2012

So, unrolling the iterator doesn't make an appreciable difference in performance (AFAICT) - at the moment I had to take that measurement on Linux/Chrome, because my Mac is acting really odd (aka SLOW.) I think this code is good to go.

danvk added a commit that referenced this pull request Jun 20, 2012

Merge pull request #155 from kberg/master
Fix behavior regression, added tests and supporting API

@danvk danvk merged commit 856c1ff into danvk:master Jun 20, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment