Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629463002:
Refactor valuesForFrameRangeImpl (Closed)
|
Created:
3 years, 11 months ago by Raymond Toy Modified:
3 years, 10 months ago Reviewers:
hongchan CC:
chromium-reviews, blink-reviews, haraken Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor valuesForFrameRangeImpl
This function is 766 lines long with complicated logic. Move each
core subsection to its own function to make the overall logic clearer.
BUG=675776
TEST=covered by existing tests
Review-Url: https://codereview.chromium.org/2629463002
Cr-Commit-Position: refs/heads/master@{#450528}
Committed: https://chromium.googlesource.com/chromium/src/+/abcc1f5eb8bb22bd603342f951dbd076c8491d3b
Patch Set 1 #Patch Set 2 : Refactor more #Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Refactor more adding tuple and AutomationState #Patch Set 6 : Make AutomationState const and add documentation #
Total comments: 14
Patch Set 7 : Address review comments #Patch Set 8 : Fix unused variable error #
Messages
Total messages: 25 (11 generated)
The CQ bit was checked by rtoy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rtoy@chromium.org changed reviewers: + hongchan@chromium.org
PTAL
Should we worried about the performance regression with std::tuple and std::tie? I don't really worry about it, but that's just me. If you think I need to go over all the new methods line by line, let me know. I am assuming you just copied/pasted the code and made them as separate methods. https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right): https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:875: // float k = deltaTime > 0 ? 1 / deltaTime : 0; These are commented out. https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:983: std::tuple<size_t, unsigned> AudioParamTimeline::handleFirstEvent( From this to the end, you just copied/pasted and separate them into different methods? or should I review the content of this method line by line? https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:1016: // Remove this line. https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h (right): https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:163: const Vector<float>& curve() const { return m_curve; } What is the benefit of this pattern? A const getter and a writable getter with the same name seem to be confusing - do we really need this? https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:269: // Parameters for the current automation request. Merge this comment with l.267? https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:282: An extra empty line. https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:392: // computed |value|, and the updated \writeIndex|. \writeIndex| => |writeIndex| + and all the following instances.
On 2017/02/10 23:37:29, hongchan wrote: > Should we worried about the performance regression with std::tuple and std::tie? > I don't really worry about it, but that's just me. Measuring this will be hard. Not exactly sure how to do it. > > If you think I need to go over all the new methods line by line, let me know. I > am assuming you just copied/pasted the code and made them as separate methods. For the most part. The first patch basically did that, and just passed in enough parameters to make allow the function to work. The remaining patches were cleaning this up and simplifying the parameters, identifying the common bits, and using tuples to make it clearer what is actually being modified by the routine. > > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right): > > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:875: // float > k = deltaTime > 0 ? 1 / deltaTime : 0; > These are commented out. > > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:983: > std::tuple<size_t, unsigned> AudioParamTimeline::handleFirstEvent( > From this to the end, you just copied/pasted and separate them into different > methods? > or should I review the content of this method line by line? > > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:1016: // > Remove this line. > > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h (right): > > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:163: const > Vector<float>& curve() const { return m_curve; } > What is the benefit of this pattern? A const getter and a writable getter with > the same name seem to be confusing - do we really need this? > > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:269: // > Parameters for the current automation request. > Merge this comment with l.267? > > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:282: > An extra empty line. > > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:392: // computed > |value|, and the updated \writeIndex|. > \writeIndex| => |writeIndex| > > + and all the following instances.
https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h (right): https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:163: const Vector<float>& curve() const { return m_curve; } On 2017/02/10 23:37:29, hongchan wrote: > What is the benefit of this pattern? A const getter and a writable getter with > the same name seem to be confusing - do we really need this? Not sure about the pattern, but a few other bits of webaudio code do this. (But other places have foo->data() and foo->mutableData().) A const function is needed because AutomationState has a const ParamEvent* and processSetValueCurve needs access to it. Without this, the compiler complains that curve() isn't a const function.
On 2017/02/10 23:42:36, Raymond Toy wrote: > On 2017/02/10 23:37:29, hongchan wrote: > > Should we worried about the performance regression with std::tuple and > std::tie? > > I don't really worry about it, but that's just me. > > Measuring this will be hard. Not exactly sure how to do it. > We can get a stack trace of AudioParamTimeline::valuesForFrameRangeImpl() and monitor the CPU weight.
On 2017/02/10 23:52:36, Raymond Toy wrote: > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h (right): > > https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:163: const > Vector<float>& curve() const { return m_curve; } > On 2017/02/10 23:37:29, hongchan wrote: > > What is the benefit of this pattern? A const getter and a writable getter with > > the same name seem to be confusing - do we really need this? > Not sure about the pattern, but a few other bits of webaudio code do this. (But > other places have foo->data() and foo->mutableData().) > > A const function is needed because AutomationState has a const ParamEvent* and > processSetValueCurve needs access to it. Without this, the compiler complains > that curve() isn't a const function. Unfortunate. I'll just leave it up to you. If you think this is unavoidable, let's leave it as it is.
On 2017/02/14 18:02:14, hongchan wrote: > On 2017/02/10 23:42:36, Raymond Toy wrote: > > On 2017/02/10 23:37:29, hongchan wrote: > > > Should we worried about the performance regression with std::tuple and > > std::tie? > > > I don't really worry about it, but that's just me. > > > > Measuring this will be hard. Not exactly sure how to do it. > > > > We can get a stack trace of AudioParamTimeline::valuesForFrameRangeImpl() and > monitor the CPU weight. I meant finding a test that exercises the timeline in a useful manner that shows up as a significant part of the profile.
On 2017/02/14 18:29:15, Raymond Toy wrote: > On 2017/02/14 18:02:14, hongchan wrote: > > On 2017/02/10 23:42:36, Raymond Toy wrote: > > > On 2017/02/10 23:37:29, hongchan wrote: > > > > Should we worried about the performance regression with std::tuple and > > > std::tie? > > > > I don't really worry about it, but that's just me. > > > > > > Measuring this will be hard. Not exactly sure how to do it. > > > > > > > We can get a stack trace of AudioParamTimeline::valuesForFrameRangeImpl() and > > monitor the CPU weight. > > I meant finding a test that exercises the timeline in a useful manner that shows > up as a significant part of the profile. On my linux z620 running https://musiclab.chromeexperiments.com/Piano-Roll which has lots of automations I get the following results using perf. ToT: AudioParamTimeline::valuesForFrameRangeImpl: 0.67% CL: AudioParamTimeline::valuesForFrameRangeImpl: 0.71% The difference might be in the noise. In any case, the overall cost of this is pretty small for the demo.
https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right): https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:875: // float k = deltaTime > 0 ? 1 / deltaTime : 0; On 2017/02/10 23:37:29, hongchan wrote: > These are commented out. Done. https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:983: std::tuple<size_t, unsigned> AudioParamTimeline::handleFirstEvent( On 2017/02/10 23:37:29, hongchan wrote: > From this to the end, you just copied/pasted and separate them into different > methods? > or should I review the content of this method line by line? Yes, I basically copied and pasted the bodies from the corresponding code. I think 1189 was the only line I had to modify from the original (because the routine recomputes nextEventType). https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:1016: // On 2017/02/10 23:37:29, hongchan wrote: > Remove this line. Done. https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h (right): https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:269: // Parameters for the current automation request. On 2017/02/10 23:37:29, hongchan wrote: > Merge this comment with l.267? Done. https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:282: On 2017/02/10 23:37:29, hongchan wrote: > An extra empty line. Done. https://codereview.chromium.org/2629463002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:392: // computed |value|, and the updated \writeIndex|. On 2017/02/10 23:37:29, hongchan wrote: > \writeIndex| => |writeIndex| > > + and all the following instances. Done.
lgtm
The CQ bit was checked by rtoy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by rtoy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hongchan@chromium.org Link to the patchset: https://codereview.chromium.org/2629463002/#ps140001 (title: "Fix unused variable error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487106998317390,
"parent_rev": "687c39deed13217382e70376304e664ea0d124b9", "commit_rev":
"abcc1f5eb8bb22bd603342f951dbd076c8491d3b"}
Message was sent while issue was closed.
Description was changed from ========== Refactor valuesForFrameRangeImpl This function is 766 lines long with complicated logic. Move each core subsection to its own function to make the overall logic clearer. BUG=675776 TEST=covered by existing tests ========== to ========== Refactor valuesForFrameRangeImpl This function is 766 lines long with complicated logic. Move each core subsection to its own function to make the overall logic clearer. BUG=675776 TEST=covered by existing tests Review-Url: https://codereview.chromium.org/2629463002 Cr-Commit-Position: refs/heads/master@{#450528} Committed: https://chromium.googlesource.com/chromium/src/+/abcc1f5eb8bb22bd603342f951db... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/abcc1f5eb8bb22bd603342f951db... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
