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.

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

A performance issue #23

Open
felixhao28 opened this issue Apr 3, 2015 · 10 comments
Open

A performance issue #23

felixhao28 opened this issue Apr 3, 2015 · 10 comments

Comments

@felixhao28
Copy link

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.

@codesuki
Copy link
Owner

codesuki commented Apr 4, 2015

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

@felixhao28
Copy link
Author

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
Copy link
Author

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

@codesuki
Copy link
Owner

codesuki commented Apr 4, 2015

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

@jeroencoumans
Copy link
Contributor

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 :)

@codesuki
Copy link
Owner

codesuki commented Apr 6, 2015

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

@felixhao28
Copy link
Author

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.

@jefffriesen
Copy link

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:

dc-js/dc.js#630 (comment)

@codesuki
Copy link
Owner

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.)

@jefffriesen
Copy link

Cool. Looking forward to seeing out transitions turn out.

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

No branches or pull requests

4 participants