Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2800213002:
Avoid duplicate functions/code in core/inspector.
|
Created:
3 years, 8 months ago by Daniel Bratell Modified:
3 years, 4 months ago Reviewers:
pfeldman CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid duplicate functions/code in core/inspector.
While experimenting with unity builds I encountered a few duplicate
symbols and functions in core/inspector. This patch moves frameId()
and urlWithoutFragment to be member functions so that they have a more
clearly defined scope. Also deleted a function declaration for a
function I could not find.
Patch Set 1 #Patch Set 2 : Rebased past blink rename #Patch Set 3 : Merge two UrlWithoutFragment() and two FrameId() functions. #
Total comments: 1
Patch Set 4 : A third way #
Total comments: 8
Patch Set 5 : Addressed review comments. #
Total comments: 1
Patch Set 6 : Manual inlined urlWithoutFragment #
Messages
Total messages: 49 (28 generated)
The CQ bit was checked by bratell@opera.com to run a CQ dry run
bratell@opera.com changed reviewers: + alph@chromium.org
PTAL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
>> This patch moves frameId() and urlWithoutFragment to be member functions so that they have a more >> clearly defined scope. How come static is better than anonymous namespace? Could you elaborate on what you are doing and why? >> Also deleted a function declaration for a function I could not find. Thanks for this!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/07 23:27:19, pfeldman wrote: > >> This patch moves frameId() and urlWithoutFragment to be member functions so > that they have a more > >> clearly defined scope. > > How come static is better than anonymous namespace? Could you elaborate on what > you are doing and why? Unity builds works by merging compilation units so code written under the assumption of a one-cc-file-only compilation unit suddenly sees itself together with other cc files, and their local functions. The main idea behind anonymous namespaces is that they are not exported beyond the compilation unit, but if there are more than one anonymous namespace in the same compilation unit then there can be collisions between them. static functions at the top level are no worse or bad in that respect. The change here is to make the functions static at the class level so that InspectionPageAgent::frameId() doesn't collide with anything at the global level or in the anonymous namespace level. The could have been non-static too but that would have been unnecessary since they need no member variables or object. As for the why I'm doing unity build experiments at all, I think I better redirect to the mail threads on blink-dev which show a 7x-14x compile speed improvement in Blink/core by using unity builds. Such an improvement would be fantastic.
The CQ bit was checked by bratell@opera.com 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.
> As for the why I'm doing unity build experiments at all, I think I better > redirect to the mail threads on blink-dev which show a 7x-14x compile speed > improvement in Blink/core by using unity builds. Such an improvement would be > fantastic. I don't think it is feasible to achieve your goal as long as the entire team is working against you and introduces more anonymous namespaces. Anonymous namespaces are encouraged practice as per https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Stat.... So you should either fork the repo for your experiment or convince chromium authors they should not use anonymous namespaces first.
On 2017/04/17 20:25:04, pfeldman wrote: > > As for the why I'm doing unity build experiments at all, I think I better > > redirect to the mail threads on blink-dev which show a 7x-14x compile speed > > improvement in Blink/core by using unity builds. Such an improvement would be > > fantastic. > > I don't think it is feasible to achieve your goal as long as the entire team is > working against you and introduces more anonymous namespaces. Anonymous > namespaces are encouraged practice as per > https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Stat.... > So you should either fork the repo for your experiment or convince chromium > authors they should not use anonymous namespaces first. Nah, I don't mind anonymous namespaces at all. They are not worse than the global namespace. It only becomes a problem if two anonymous namespaces in the same gn target uses the same symbol names which is actually really rare. There are other good reasons to keep names unique (grep, debugger, crash stacks) which probably contributes. A quick estimate is that there was one conflict per 50-100 files. editor and inspector had slightly above that rate. Most code had no conflicts at all. https://codereview.chromium.org/2824663002/ is all the changes needed to get jumbo builds in Blink. Not very much as you can see.
I think I understand where you are coming from. But it seems very easy to regress the invariant you are introducing. Could you conduct your experiment on a branch, get consensus on whether you are going for it or not and then we'll introduce the new policy and fix the outliers? Wdyt?
On 2017/04/18 17:42:14, pfeldman wrote: > I think I understand where you are coming from. But it seems very easy to > regress the invariant you are introducing. Could you conduct your experiment on > a branch, get consensus on whether you are going for it or not and then we'll > introduce the new policy and fix the outliers? Wdyt? I have done the experiments (numbers available on blink-dev), but I'm trying to remove as many special cases as possible so that people will be able to see the forest and not the trees. And to make the branch smaller and easier to maintain. So far the response to my blink-dev posts about this has been positive from non-Googlers and infra people and silence from the rest. I hope to make it so simple to just try it that people will test it. You are welcome to try the patch in the CL above (especially if you are on mac since there is a mac compile error I could use some help understanding. :-) )! My goal for the small patches has been to make them self sustained and meaningful regardless of whether there will be official jumbo builds or not. This one, which is not about duplicated code like most of them, is mostly a question of taste but I want the reused name to go away in some way. Can change names or scope or inline or if nothing makes things better, we can leave this file excluded from jumbo compiling.
>> My goal for the small patches has been to make them self sustained and >> meaningful regardless of whether there will be official jumbo builds or not. There is still nothing preventing me from regressing what you are doing, that's my major concern. I'll add two more anonymous namespaces with toString methods tomorrow. Let's get better understanding on whether this is going somewhere. If it is, I'll be happy to review or even assist you doing it.
Description was changed from ========== Avoid duplicate functions/code in core/inspector. While experimenting with unity builds I encountered a few duplicate symbols and functions in core/inspector. This patch moves frameId() and urlWithoutFragment to be member functions so that they have a more clearly defined scope. Also deleted a function declaration for a function I could not find. ========== to ========== Avoid duplicate functions/code in core/inspector. While experimenting with unity builds I encountered a few duplicate symbols and functions in core/inspector. This patch moves frameId() and urlWithoutFragment to be member functions so that they have a more clearly defined scope. Also deleted a function declaration for a function I could not find. ==========
alph@chromium.org changed reviewers: - alph@chromium.org
On 2017/04/18 20:24:15, pfeldman wrote: > >> My goal for the small patches has been to make them self sustained and > >> meaningful regardless of whether there will be official jumbo builds or not. > > There is still nothing preventing me from regressing what you are doing, that's > my major concern. I'll add two more anonymous namespaces with toString methods > tomorrow. > Let's get better understanding on whether this is going somewhere. If it is, > I'll be happy to review or even assist you doing it. An update: Since my comment I have had a lot of great results from people at Google testing out jumbo builds and I have raised the question about how to make it official, but I don't expect any responses this week since so many people are away. I don't agree with your statement that a change that can be regressed is a major concern. There is a ton of changes happening all the time which could regress without being detected by cq and that doesn't stop us from making them. If on the other hand the changes are bad or make something worse I would be happy to address this in some other way. You know the code and its intended structure so much better than I so I would not be surprised if I choose a suboptimal approach.
The CQ bit was checked by bratell@opera.com 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...
pfeldman, here is another approach. Instead of class static functions, I've moved UrlWithoutFragment to the closest common base class header and added it as a utility function there, and the null checking IdentifiersFactory::FrameId() I moved to IdentifiersFactory and called FrameIdSafe(). Does this seem like a better way to get rid of the duplicate functions?
bratell@opera.com changed reviewers: + pfeldman@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2800213002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorBaseAgent.h (right): https://codereview.chromium.org/2800213002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorBaseAgent.h:113: KURL UrlWithoutFragment(const KURL&); This really is not a great place for this method. I would not look for it here.
On 2017/07/05 18:24:55, pfeldman wrote: > https://codereview.chromium.org/2800213002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/inspector/InspectorBaseAgent.h (right): > > https://codereview.chromium.org/2800213002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectorBaseAgent.h:113: KURL > UrlWithoutFragment(const KURL&); > This really is not a great place for this method. I would not look for it here. Need your help here. What would you do? There are so many ways to solve it and I don't know what you, who own and live with this code, would prefer or like.
The CQ bit was checked by bratell@opera.com 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...
pfeldman, can you please take a look at this variant? I guessed from your last review that the FrameIdSafe part was fine and that you only wanted a better home for UrlWithoutFragment. This makes AddStringToDigestor.cpp into a general Helpers file (with a new name of course).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/IdentifiersFactory.cpp (right): https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/IdentifiersFactory.cpp:62: return frame ? FrameId(frame) : ""; Bake it into FrameId(), return String() if nullptr is passed. https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorHelpers.cpp (right): https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorHelpers.cpp:14: const CString c_string = string.Utf8(); Could you just inline this? https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorHelpers.cpp:20: KURL result = url; ditto
The CQ bit was checked by bratell@opera.com 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...
Addressed the review comments. Please take a new look. https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/IdentifiersFactory.cpp (right): https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/IdentifiersFactory.cpp:62: return frame ? FrameId(frame) : ""; On 2017/07/21 18:10:59, pfeldman wrote: > Bake it into FrameId(), return String() if nullptr is passed. Done. https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorHelpers.cpp (right): https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorHelpers.cpp:14: const CString c_string = string.Utf8(); On 2017/07/21 18:10:59, pfeldman wrote: > Could you just inline this? If "string.Utf8()" is moved to be inside the function calls the conversion to UTF-8 will be executed twice. Once to pass a pointer, once to calculate the length of UTF-8 string. Or did you intend to comment on the whole function and wanted it to move to the header? That would be against the style guide ( https://www.chromium.org/developers/coding-style/cpp-dos-and-donts ). https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorHelpers.cpp:20: KURL result = url; On 2017/07/21 18:10:59, pfeldman wrote: > ditto Inline result? There is a non-const destructive function called on it so it's not just to move it but I changed the arguent to a url copy instead of a reference. Saved one line. Or do you mean inlining the whole method? (Your comment is on the local variable but maybe that is just off by one). I think that would be against the style guide as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorHelpers.cpp (right): https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorHelpers.cpp:14: const CString c_string = string.Utf8(); I wanted to inline these two lines into both call sites. https://codereview.chromium.org/2800213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorHelpers.cpp:20: KURL result = url; Inline entire method: KURL nofragment = url; nofragment.RemoveFragmentIdentifier(); in both call sites. https://codereview.chromium.org/2800213002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/IdentifiersFactory.h (right): https://codereview.chromium.org/2800213002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/IdentifiersFactory.h:48: static String FrameIdSafe(LocalFrame*); You no longer need this.
The CQ bit was checked by bratell@opera.com 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...
There was a bit more than two places using that function but this is what the patch looks like with it manually inlined. I guess you better select which version you think is best.
On 2017/07/26 10:38:39, Daniel Bratell wrote: > There was a bit more than two places using that function but this is what the > patch looks like with it manually inlined. I guess you better select which > version you think is best. Oh, and I'll make another patch with manually inlined AddStringToDigestor. This review is already long and messy enough.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
pfeldman, can you take a new look?
Looking at the URL without fragment, it clearly makes the code less readable due to inlining. And you don't let us extract these helper functions because they clash. Is there a doc or a decision that your project has enough priority to force changes like this? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
