search

catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.93k stars 563 forks source link

Cache getSortedInputEvents #1654

Closed benshayden closed 9 years ago

benshayden commented 9 years ago

A huge fraction of findInputInteractionRecords is spent iterating over the entire model over and over and over and over... while the few input events can easily be cached. C.f. https://github.com/catapult-project/catapult/issues/1626 C.f. https://crash-staging.corp.google.com/browse?q=reportid=%27861a4adf5fe0a67e%27#6

@natduca You objected to this initially. Do you still have strong feelings about it?

natduca commented 9 years ago

Can you share more consolidated data, instead of a single link? And explain why we iterate over and over? Detailed bug explanations that provide explanation of what is happening for non-experts help build consensus to your proposal without making the reviewers of the bug have to go figure out the answer for themselves.

First, the linked flamechart is incredibly hard to consult [we don't use those anymore, why isn't it a newer viewer? and, why do we have it on crash in the first place?].

Second, what number is a huge fraction? What dataset were you using?

Third, one problem with GetSortedInputEvents is that its not model specific. We have model_indices.html that could concievably have a cache of things, but we don't have a mechanism for caching stuff that is chrome specific yet.

Fourth, we could possibly dangle chrome-specific caching off of the chrome_model_helper, which may make sense. But, i know that we reconstruct the helper every time we need it. Would that be a problem? I've thought of in the past making it so that the model has a GetHelperOfTypeIfPossbible(typeName) that creates a helper if it doesn't exist, and then returns it. Maybe we need that?

So in closing, no prior choice or opinion I've had on perf is immutable given a high enough quality analysis and explanation. The key with these things is to explore and explain the space excellently: show that you have a mastery of the problem with a clear data and text explanations. Then explain carefully what design options exist for doing better, and their upside/downside. When you have done that, then the decision will be easy. :)

benshayden commented 9 years ago

Every time that findInputInteractionRecords is called, getSortedInputEvents is called 10 times. There are 784 v8 samples in findInputInteractionRecords in the linked v8 trace.

handleMouseDragEvents is 123 samples, 45 of which are in getSortedInputEvents. handleFlingEvents is 106 samples, 50 of which are in getSortedInputEvents. handleTouchEvents is 68 samples, all 68 of which are in getSortedInputEvents. There were no touch events, but getSortedInputEvents took the same amount of time as each of the other 9 calls. handleScrollEvents is 58 samples, all 58 of which are in getSortedInputEvents. There were no scroll events, but getSortedInputEvents took the same amount of time as each of the other 9 calls. handlePinchEvents is 49 samples, all 49 of which are in getSortedInputEvents. There were no pinch events, but getSortedInputEvents took the same amount of time as each of the other 9 calls. handleKeyboardEvents is 49 samples, all 49 of which are in getSortedInputEvents. There were no keyboard events, but getSortedInputEvents took the same amount of time as each of the other 9 calls. handleMouseWheelEvents is 49 samples, all 49 of which are in getSortedInputEvents. There were no mouse wheel events, but getSortedInputEvents took the same amount of time as each of the other 9 calls. handleMouseResponseEvents is 47 samples, all 47 of which are in getSortedInputEvents. There were no mouse response events, but getSortedInputEvents took the same amount of time as each of the other 9 calls. handleTapResponseEvents is 47 samples, all 47 of which are in getSortedInputEvents. There were no tap response events, but getSortedInputEvents took the same amount of time as each of the other 9 calls. checkAllInputEventsHandled is 53 samples, 51 of which are in getSortedInputEvents.

Of the 784 samples in findInputInteractionRecords, 631 of them, or 80.485%, are in getSortedInputEvents.

Caching the input events costs 7 more lines of code, with no new concepts.

benshayden commented 9 years ago

The alternative to caching input events is going straight to CrBrowserMain's asyncSliceGroup rather than iterating over every thread in every process, but that container contains 3672 slices in trace_aol, which is 10 times more than the 377 that are input events. Even when getSortedInputEvents only iterates over CrBrowserMain's asyncSliceGroup, caching the input events would still save 9 iterations over thousands of slices that it already knows it's going to ignore.

benshayden commented 9 years ago

I admit that I don't understand these points:

Third, one problem with GetSortedInputEvents is that its not model specific. We have model_indices.html that could concievably have a cache of things, but we don't have a mechanism for caching stuff that is chrome specific yet.

Fourth, we could possibly dangle chrome-specific caching off of the chrome_model_helper, which may make sense. But, i know that we reconstruct the helper every time we need it. Would that be a problem? I've thought of in the past making it so that the model has a GetHelperOfTypeIfPossbible(typeName) that creates a helper if it doesn't exist, and then returns it. Maybe we need that?

Those points seem to be needlessly complicating what could be a simple fix. If we need a more general mechanism, then I'll be happy to help with it and I'm not trying to get in its way, but I don't see that it's needed, much less that it needs to block this bug fix.

natduca commented 9 years ago

Those points seem to be needlessly complicating what could be a simple fix

I thought you were proposing caching the input events on the model. You will note in your original bug description that you were not specific about where the caching was going to be performed, so I used my best memory of the situation and assumed you would want to cache on the model.

The quality of the feedback will get from me is proportional to the quality of the information you give to me in solicitation of that feedback. Had I been reminded of where you intended to cache in the original bug, I would not have spent time trying to write up how to address issues in caching location. And you would not be confused either.

What would have worked great, for example, would be this: "N ms of time in the aol_trace is spent in findInputInteractionRecords. Within that, we call getSortedInputEvents M times, for a total cost of N ms. If we cache getSortedInputEvents on the RAILIRFinder class, we could speed this up."