Drawing perf take2 #1690
Drawing perf take2 #1690
Conversation
- so that Drawing.bBox does not have to query the DOM to fing them on every call.
- to not have to instantiate one on every appendSVG call
src/components/drawing/index.js
Outdated
| gd._tester = tester; | ||
| gd._testref = testref; | ||
| drawing.tester = gd._tester = tester; | ||
| drawing.testref = gd._testref = testref; |
There was a problem hiding this comment.
gd._testref currently isn't used anywhere so we might as well get rid of it before someone does use it.
gd._tester is used in a few places, but wouldn't be hard to 🔪 and replace with drawing.tester.
There was a problem hiding this comment.
🛑 Hold the presses 🛑
Caching the tester div and testref path on the Drawing module object will make plotly.js even more broken in child windows than it is at the moment. From #702 and the attempted PR #764, plotly.js CSS is inserted once per <html> page - which doesn't propagate to child windows. We should instead insert plotly.js CSS once per document.
Similarly, we should be caching tester and testref once per document too. That is, child windows need their own tester nodes. As we don't have access to cache once-per-document object (adding refs to document sounds dirty), caching tester and testref on the gd might actually be the best we can do.
This would imply passing gd to Drawing.bBox below - which isn't a high price to pay.
There was a problem hiding this comment.
child windows need their own tester nodes
Do they? It looks to me (fiddling in the codepen from @nielsenb-jf ) as though if you have a reference to a node, you can interact with it fully even if it's in a different window. It won't get the same CSS applied to it, sure, but we don't depend on that here, do we?
There was a problem hiding this comment.
Right, you can interact with the base document in a child window. But does getBoundingClientRect() give you the correct answer in the case?
There was a problem hiding this comment.
See what happens when setting layout.title:
http://codepen.io/etpinard/pen/LyBQLj?editors=0010
gives
which I believe is due to Drawing.bBox not computing the correct values.
There was a problem hiding this comment.
@alexcjohnson can we agree on fixing the child window behavior in an another PR?
I'm ok leaving this PR as is or pushing two more commits replacing gd._tester -> Drawing.tester and gd._testref -> Drawing.testref.
There was a problem hiding this comment.
Sure. child window behavior is strange... like it looks like in IE and Edge we have some even bigger problems with it.
FWIW I think the layout.title issue you flagged above is not a bounding box issue, but the fact that the two <svg> layers are somehow not on top of each other, the front layer appears down below the main one.
There was a problem hiding this comment.
re: Drawing. vs gd. - we had better pick one and use only that. As long as we look into this issue in the near future I don't care too much which we use. As is, if there is any difference between them, we'll be making a mess of it as soon as we have plots in two different windows... but in an order-dependent way that will be very hard to debug.
There was a problem hiding this comment.
Ok. Let's go with Drawing 👍
src/lib/svg_text_utils.js
Outdated
| @@ -17,6 +17,17 @@ var Lib = require('../lib'); | |||
| var xmlnsNamespaces = require('../constants/xmlns_namespaces'); | |||
| var stringMappings = require('../constants/string_mappings'); | |||
|
|
|||
| exports.getDOMParser = function() { | |||
| if(exports.domParser) { | |||
| return exports.domParser; | |||
There was a problem hiding this comment.
Lets make domParser a module-level global instead of an export - it's unsafe to use in general as it might not exist before calling exports.getDOMParser().
There was a problem hiding this comment.
Good call. Done in e0050f7
|
LGTM! 💃 |
|
@etpinard I did some quick test and did not find any difference other than some minor variance. So it should be okay in terms of performance. Thanks for checking with me! |
|
@hy9be thank you for checking 👌 |
fixes #1675
Take 2 of 29abb5d from @hy9be's #1585 to be merged in #1683
@hy9be would you mind checking that this solution here is as fast as yours?