react-d3-components icon indicating copy to clipboard operation
react-d3-components copied to clipboard

A performance issue

Open felixhao28 opened this issue 10 years ago • 10 comments

Will you consider calling tooltipLine method in a delayed fashion? Since it is the only access to the selected data and I need to do a lot of time-consuming things with that. It significantly reduced the UI responsiveness.

When I say delayed fashion, I mean this:

before:

onMouseEnter: (e, data) ->
    tooltip = @props.tooltipLine(data, position)
    @setState
        tooltip: tooltip

after:

onMouseEnter: (e, data) ->
    if @delayedTooltip?
        clearTimeout @delayedTooltip
    @delayedTooltip = setTimeout =>
        tooltip = @props.tooltipLine(data, position)
        @setState
            tooltip:tooltip
        @delayedTooltip = null

Actually, I can just put my heavy work in a setTimeout callback. And that is what I did, but it took me hours to realize that. I think it might be a good practice to put every callback function in this fashion, because who knows what user will do.

felixhao28 avatar Apr 03 '15 11:04 felixhao28

Thanks for the feedback! I will consider it. What are you doing in the tooltip function that takes so much time?

codesuki avatar Apr 04 '15 00:04 codesuki

Since I would like to "preview" the selected data in other components, I simply set the data into the state of the father component. And a change to that data will cause all related components to update. So if user moves mouse too fast, the page will be just too busy handling all these state updating to respond to incoming mouse move events.

felixhao28 avatar Apr 04 '15 07:04 felixhao28

Honestly, I just use tooltipLine function as "onSelectedDataChange" event handler.

felixhao28 avatar Apr 04 '15 07:04 felixhao28

I see, I was thinking about changing the mouse stuff anyhow since your Issue about events. So I might use RxJS and debounce it!

codesuki avatar Apr 04 '15 14:04 codesuki

Please don't put this in the library as a standard feature. I'd expect most people don't do expensive stuff in their tooltip handlers, and it's easy enough to handle this yourself as the OP demonstrated :)

jeroencoumans avatar Apr 05 '15 13:04 jeroencoumans

True. Ultimately I want it to be broadly usable without too much forced standards etc, so in case I change it, should be optional.

codesuki avatar Apr 06 '15 02:04 codesuki

I just learned that the official name for this is "debounce". It seems to be a widely applied technique to prevent browser-based JavaScript applications from firing events too often that it bricks UI responsiveness.

Just put it here in case someone needs further information.

Edit: Oh and I just noticed that codesuki mentioned this in an earlier reply. Did not catch the meaning of that at the first time.

felixhao28 avatar Apr 22 '15 06:04 felixhao28

Also keep in mind the difference between debounce and throttle. I would have to think more about which one is appropriate in this case. Here's a thread with resources on it from dc.js:

https://github.com/dc-js/dc.js/issues/630#issuecomment-94202879

jefffriesen avatar Apr 22 '15 13:04 jefffriesen

Thanks @jefffriesen for the info I will have a look.

BTW the library is still evolving. I am almost finished adding transition support, the problem I have to solve now is to transition not only for example the line graph but also the labels and tooltip. I use this chance to rethink the components and tooltips, too (as talked about in the the other issues.)

codesuki avatar Apr 23 '15 06:04 codesuki

Cool. Looking forward to seeing out transitions turn out.

jefffriesen avatar Apr 23 '15 23:04 jefffriesen