Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Plots.resize: promises should always be resolved #4392

Merged
merged 5 commits into from
Dec 5, 2019
Merged

Plots.resize: promises should always be resolved #4392

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
deb32ff
Plots.resize: reject old promises
antoinerg Nov 29, 2019
040d6ac
Plots.resize: resolve old promises
antoinerg Dec 4, 2019
5ee431e
Plots.resize: only resolve if a new request hasn't been queued
antoinerg Dec 4, 2019
5e71cff
Plots.resize: delete old promise resolver if present
antoinerg Dec 5, 2019
14f518f
Plots.resize: improve test
antoinerg Dec 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,15 @@ plots.redrawText = function(gd) {
plots.resize = function(gd) {
gd = Lib.getGraphDiv(gd);

return new Promise(function(resolve, reject) {
var resolveLastResize;
var p = new Promise(function(resolve, reject) {
if(!gd || Lib.isHidden(gd)) {
reject(new Error('Resize must be passed a displayed plot div element.'));
}

if(gd._redrawTimer) clearTimeout(gd._redrawTimer);
if(gd._resolveResize) resolveLastResize = gd._resolveResize;
gd._resolveResize = resolve;

gd._redrawTimer = setTimeout(function() {
// return if there is nothing to resize or is hidden
Expand All @@ -101,10 +104,14 @@ plots.resize = function(gd) {

Registry.call('relayout', gd, {autosize: true}).then(function() {
gd.changed = oldchanged;
resolve(gd);
// Only resolve if a new call hasn't been made!
if(gd._resolveResize === resolve) resolve(gd);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Took me a while and drawing some timing diagrams to be sure I agreed with this, but I think you've got it 🏆

Only thing I might suggest is when we finally get to this resolve(gd), should we also delete gd._resolveResize? Otherwise the next time someone resizes 10 minutes later, there will still be a resolveLastResize(p) call. In principle it's OK to resolve the same promise more than once, but it feels nicer not to, also then we don't need to worry about it during purge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old promise resolver is now deleted in 5ee431e !

});
}, 100);
});

if(resolveLastResize) resolveLastResize(p);
return p;
};


Expand Down
18 changes: 18 additions & 0 deletions test/jasmine/tests/plots_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,24 @@ describe('Test Plots', function() {
.then(done);
});
});

describe('returns Promises', function() {
afterEach(destroyGraphDiv);

it('should resolve them all', function(done) {
gd = createGraphDiv();
var p = [];
Plotly.newPlot(gd, [{y: [5, 2, 5]}])
.then(function() {
p.push(Plotly.Plots.resize(gd));
p.push(Plotly.Plots.resize(gd));
p.push(Plotly.Plots.resize(gd));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Thanks for pointing this out. I'm wondering though, why should the second Plots.resize call result in a promise-reject? Could it resolve and maybe Lib.log resize in progress instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why should the second Plots.resize call result in a promise-reject? Could it resolve and maybe Lib.log resize in progress instead?

I thought that rejecting made more sense since the resize has not happened yet. The goal is to clearly tell the caller that the operation is not completed. Listening to the log is probably not ideal for this purpose.

Another possibility would be to resolve all pending promises once the resize operation is done. We could keep all the resolve functions from previous promises in an array and simply call them all when resizing is done.

I'm not sure what's the best behavior to adopt here.

cc @Marc-Andre-Rivet @alexcjohnson

Copy link

@Marc-Andre-Rivet Marc-Andre-Rivet Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that rejecting made more sense since the resize has not happened yet.

As a customer of the promise both resolving or rejecting the promise can make sense. If resolving instead of rejecting, providing a state that makes it clear that a resize has occurred or not would be necessary to add value vs. rejection: callers don't need to keep track of the number of in-progress requests anymore, they only need to wait for a promise to resolve with a success state.

--Actually if resize can take longer than the debounce timeout, true/false state on resolve will not be of any use. Seems pretty clear this could happen, which means a counter from the caller's side is still the best bet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so according to @Marc-Andre-Rivet, it seems like resolving/rejecting versus resolving true/ false is equivalent.

It is, therefore, a matter of preference/philosophy for @plotly/plotly_js. I don't have a strong preference as long as all promises are fulfilled or rejected. Just let me know what you would prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A caller probably doesn't actually care that another resize delayed the redraw, they just want to know when the resize is really truly done, even if it takes 10 sec because someone keeps spamming resize calls. So what if instead we resolve all of these promises at the same time, whenever the redraw really happens?

If I'm understanding this correctly, we can do that by simply resolving the first promise with the new promise we've just made - ie store it in a var rather than just return new Promise(...) (sorry if the below is obvious to you, I had to try it to make sure it worked as I thought it should)

> p=new Promise((resolve, reject) => {window.res=resolve})
> p.then(v=>console.log('p: ', v))
> p2=new Promise((resolve, reject) => {window.res2=resolve})
> p2.then(v=>console.log('p2: ', v))
> res(p2) // this doesn't resolve p, because we're resolving with another promise
> p
< Promise {<pending>}
> res2(42) // this resolves p2 AND p
    p2:  42
    p:  42
> p
< Promise {<resolved>: 42}

Copy link

@Marc-Andre-Rivet Marc-Andre-Rivet Dec 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcjohnson Nice! I was not aware of that impl detail for resolve. Just to be certain, checked that the behavior is not a Chrome idiosyncracy and is implemented the same way in the promise-polyfill. My concern was about book keeping but with this approach the gd can keep track by itself.

Here's a scenario where we may need to do more than wait for a single resize operation to be triggered and completed in order to resolve the promise? Doing away with counters on the caller's side entirely.

Given that resize debounces/trails if called within 100ms and that it's possible for a resize operation to take >100ms, assuming the resize operation is at least partially async and not debounced inside

plotly.js/src/plots/plots.js

Line 105 in deb32ff

Registry.call('relayout', gd, {autosize: true}).then(function() {
and that a resizes on the same gd keep order:

[0ms] resize request No.1
[90ms] resize request No.2 within 100ms of No.1, debounces No.1
[190ms] 100ms passed, resize calc starts for No.2, this calc will take 250ms
[200ms] resize request No.3 110ms later not debouncing No.2
[300ms] 100ms passed, resize calc starts for No.3, this call will take 250ms
(speculative implementation)
[440ms] resize completes for call No.2; promise resolves to No.3 as another resize is in progress
[550ms] resize completes for call No.3; promise resolves as no other resize is in progress
[550ms] caller's resize promises for requests No.1, No.2, No.3 resolve

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call @Marc-Andre-Rivet - this is a really helpful scenario.

There's not very much that happens async in plotly.js, and in fact in a resize I'd think the vast majority of these would already be done. For the most part it's MathJax and map data, and once you've drawn the graph once, on a resize you already have the correct data so these operations should complete synchronously. But there may be some edge cases where it's still async...

Then I think the important thing is that any new call to Plotly.plot (the underlying routine that resize, relayout, react, newPlot etc all call out to) starts its async-friendly portion by waiting for any in-progress plotting promises to resolve. So I believe your request No.3 won't start its 250ms redraw until No.2 finishes its redraw - finishing at 440+250=690ms. At that point the question is, can we prevent No.2 from resolving until No.3 completes, and do we want to? I could see arguments either way - maybe you want to maintain a 1:1 correspondence between redraws and resolves, so that if there's some quick operation to do elsewhere on the page to keep visually in sync with the graph, you can have that happen each time... on the other hand it seems like it would simplify external handling a lot if all overlapping requests resolve simultaneously, as soon as you know that every queued redraw is really truly done. I guess the latter sounds better to me, a single final resolve of everything, unless anyone can think of a good argument for the former.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 040d6ac resolves all promises instead of rejecting them.

About overlapping requests:

At that point the question is, can we prevent No.2 from resolving until No.3 completes

I am not familiar with that part of the code so I can't say how much work it would involve. Maybe @etpinard could give us an idea.

cc @alexcjohnson @Marc-Andre-Rivet

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should all be manageable from within plots.resize. Seems to me we'd just have to track the Promise we're creating (attaching it to gd?), and if you're in the relayout.then(...) and another resize promise has already been started (gd._resizePromise !== myResizePromise or something?), resolve with that one rather than gd?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the following is simpler and does the right thing: 5ee431e

See the pattern in action at https://codepen.io/antoinerg/pen/YzPXyJv?editors=1002

return Promise.all(p);
})
.catch(failTest)
.then(done);
});
});
});

describe('Plots.purge', function() {
Expand Down