Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
111833006:
Avoiding multiple text/string parsing while creating TextRun (Closed)
|
Created:
7 years ago by h.joshi Modified:
6 years, 11 months ago CC:
blink-reviews, jamesr, krit, zoltan1, dsinclair, bemjb+rendering_chromium.org, eae+blinkwatch, leviw+renderwatch, danakj, Rik, f(malita), jchaffraix+rendering, Stephen Chennney, pdr., rwlbuis, leviw_travelin_and_unemployed Base URL:
https://chromium.googlesource.com/chromium/blink.git@FontOptPatch1 Visibility:
Public. |
DescriptionAvoiding multiple text/string parsing while creating TextRun. When TextRun is created we parse Text String but we are not storing CodePath. In InlineTextBox check was missing for Simple Code Path due to which multiple calls were made to "CodePath" to get code path for current text. Plus when
Run is made with complete string we can store CodePath and use the same.
Bug=330502
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165203
Patch Set 1 #Patch Set 2 : Fixing review comments for rebasing files #
Total comments: 13
Patch Set 3 : Fixing comments given by eseidel #Patch Set 4 : Fixing compile error for last commit #Patch Set 5 : Comment fixing and Code Merge #Patch Set 6 : Fixing merge issues after LGTM #
Messages
Total messages: 23 (0 generated)
Please create a bug. Also, you will need to rebase as many of the files you are modifying have moved.
On 2013/12/11 13:07:38, Stephen Chenney wrote: > Please create a bug. > > Also, you will need to rebase as many of the files you are modifying have moved. Hi, Updating code with rebasing files and also created bug for the same.
On 2013/12/23 10:26:11, h.joshi wrote: > On 2013/12/11 13:07:38, Stephen Chenney wrote: > > Please create a bug. > > > > Also, you will need to rebase as many of the files you are modifying have > moved. > > Hi, > Updating code with rebasing files and also created bug for the same. Hi Stephen, Pls review patch, information regarding CLA agreement its pending on Google side so in case patch is good to go requesting pls do not land patch before CLA is executed. Will update CLA status as soon I get update.
Emil is your best reviewer by far. https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderText.cpp (right): https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderText.cpp:59: CodePath someCodePath; nit: I would have called this "codePath". https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderText.cpp:1810: bool RenderText::computeCanUseSimpleFontCodePath() We could also just have made codepath mutable. Unclear which is better. https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderText.h (right): https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderText.h:147: CodePath getCodePath() const { return m_textCodePath;} Blink doesn't prefix simple getters with "get" https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/sv... File Source/core/rendering/svg/SVGTextMetricsBuilder.cpp (right): https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/sv... Source/core/rendering/svg/SVGTextMetricsBuilder.cpp:105: CodePath tmpPath = scaledFont.codePath(m_run); No need to prefix this with "tmp" since it's the only codePath used in thsi funtion. https://codereview.chromium.org/111833006/diff/20001/Source/platform/fonts/Fo... File Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/111833006/diff/20001/Source/platform/fonts/Fo... Source/platform/fonts/Font.cpp:184: if (codePathToUse != ComplexPath&& typesettingFeatures() && (runInfo.from || runInfo.to != runInfo.run.length())) nit: missing space before && https://codereview.chromium.org/111833006/diff/20001/Source/platform/text/Tex... File Source/platform/text/TextPath.h (right): https://codereview.chromium.org/111833006/diff/20001/Source/platform/text/Tex... Source/platform/text/TextPath.h:36: enum CodePath { AutoPath, SimplePath, ComplexPath, SimpleWithGlyphOverflowPath }; Thank you for changing these to have a *Path prefix, but I might have done that in a separate change first, to keep this one as small as possible.
https://codereview.chromium.org/111833006/diff/20001/Source/platform/graphics... File Source/platform/graphics/TextRun.cpp (right): https://codereview.chromium.org/111833006/diff/20001/Source/platform/graphics... Source/platform/graphics/TextRun.cpp:2: * Copyright (C) 2011 Apple Inc. All rights reserved. I suspect you mean Samsung 2013 here ;)
On 2013/12/27 04:36:44, eseidel wrote: > Emil is your best reviewer by far. > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... > File Source/core/rendering/RenderText.cpp (right): > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... > Source/core/rendering/RenderText.cpp:59: CodePath someCodePath; > nit: I would have called this "codePath". > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... > Source/core/rendering/RenderText.cpp:1810: bool > RenderText::computeCanUseSimpleFontCodePath() > We could also just have made codepath mutable. Unclear which is better. > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... > File Source/core/rendering/RenderText.h (right): > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... > Source/core/rendering/RenderText.h:147: CodePath getCodePath() const { return > m_textCodePath;} > Blink doesn't prefix simple getters with "get" > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/sv... > File Source/core/rendering/svg/SVGTextMetricsBuilder.cpp (right): > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/sv... > Source/core/rendering/svg/SVGTextMetricsBuilder.cpp:105: CodePath tmpPath = > scaledFont.codePath(m_run); > No need to prefix this with "tmp" since it's the only codePath used in thsi > funtion. > > https://codereview.chromium.org/111833006/diff/20001/Source/platform/fonts/Fo... > File Source/platform/fonts/Font.cpp (right): > > https://codereview.chromium.org/111833006/diff/20001/Source/platform/fonts/Fo... > Source/platform/fonts/Font.cpp:184: if (codePathToUse != ComplexPath&& > typesettingFeatures() && (runInfo.from || runInfo.to != runInfo.run.length())) > nit: missing space before && > > https://codereview.chromium.org/111833006/diff/20001/Source/platform/text/Tex... > File Source/platform/text/TextPath.h (right): > > https://codereview.chromium.org/111833006/diff/20001/Source/platform/text/Tex... > Source/platform/text/TextPath.h:36: enum CodePath { AutoPath, SimplePath, > ComplexPath, SimpleWithGlyphOverflowPath }; > Thank you for changing these to have a *Path prefix, but I might have done that > in a separate change first, to keep this one as small as possible. Hi, I have created new patch "https://codereview.chromium.org/116423007/", this patch only has *Path prefix changes as suggested.
On 2013/12/27 10:20:56, h.joshi wrote: > On 2013/12/27 04:36:44, eseidel wrote: > > Emil is your best reviewer by far. > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... > > File Source/core/rendering/RenderText.cpp (right): > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... > > Source/core/rendering/RenderText.cpp:59: CodePath someCodePath; > > nit: I would have called this "codePath". > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... > > Source/core/rendering/RenderText.cpp:1810: bool > > RenderText::computeCanUseSimpleFontCodePath() > > We could also just have made codepath mutable. Unclear which is better. > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... > > File Source/core/rendering/RenderText.h (right): > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... > > Source/core/rendering/RenderText.h:147: CodePath getCodePath() const { return > > m_textCodePath;} > > Blink doesn't prefix simple getters with "get" > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/sv... > > File Source/core/rendering/svg/SVGTextMetricsBuilder.cpp (right): > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/sv... > > Source/core/rendering/svg/SVGTextMetricsBuilder.cpp:105: CodePath tmpPath = > > scaledFont.codePath(m_run); > > No need to prefix this with "tmp" since it's the only codePath used in thsi > > funtion. > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/platform/fonts/Fo... > > File Source/platform/fonts/Font.cpp (right): > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/platform/fonts/Fo... > > Source/platform/fonts/Font.cpp:184: if (codePathToUse != ComplexPath&& > > typesettingFeatures() && (runInfo.from || runInfo.to != runInfo.run.length())) > > nit: missing space before && > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/platform/text/Tex... > > File Source/platform/text/TextPath.h (right): > > > > > https://codereview.chromium.org/111833006/diff/20001/Source/platform/text/Tex... > > Source/platform/text/TextPath.h:36: enum CodePath { AutoPath, SimplePath, > > ComplexPath, SimpleWithGlyphOverflowPath }; > > Thank you for changing these to have a *Path prefix, but I might have done > that > > in a separate change first, to keep this one as small as possible. > > Hi, > I have created new patch "https://codereview.chromium.org/116423007/", this > patch only has *Path prefix changes as suggested. Hi, Can you pls add "Emil" in list, I tried to add but not sure which one to add.
On 2013/12/27 04:46:31, Levi wrote: > https://codereview.chromium.org/111833006/diff/20001/Source/platform/graphics... > File Source/platform/graphics/TextRun.cpp (right): > > https://codereview.chromium.org/111833006/diff/20001/Source/platform/graphics... > Source/platform/graphics/TextRun.cpp:2: * Copyright (C) 2011 Apple Inc. All > rights reserved. > I suspect you mean Samsung 2013 here ;) Hi Levi, Will be deleting "TextRun.cpp" as path for this file is now modified. Will fix this.
Adding eae :)
Overall this change looks good but I do have a couple of questions and concerns. https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/In... File Source/core/rendering/InlineTextBox.cpp (right): https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:1526: run.setCharacterScanForCodePath(!textRenderer->canUseSimpleFontCodePath()); Why do we only need to do this for the complex code path? https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderText.cpp (right): https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderText.cpp:1811: { This method didn't use to have a side effect and now it does, have you ensured that this is _always_ called now (to set m_textCodePath) and that additional calls use the new codePath getter instead? https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderText.h (right): https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderText.h:147: CodePath getCodePath() const { return m_textCodePath;} Like Eric said, this should be "CodePath codePath() const {"
Also, please update the change description to better explain what you are changing and why. Finally, if you can please add a unit test for this.
Pls take a look at comments added. https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/In... File Source/core/rendering/InlineTextBox.cpp (right): https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:1526: run.setCharacterScanForCodePath(!textRenderer->canUseSimpleFontCodePath()); On 2013/12/30 19:29:14, eae wrote: > Why do we only need to do this for the complex code path? We can have three cases for text, One; String with only Simple Code point, Second String with Only Complex Code Point and third String with Mix of Simple and Complex Code points. While creating Text Object we parse the string and store if its Simple of not. As for other two cases Blink share "to" and "from" location and we need to judge string Code Points based on "to" and "from". For case 1 it will always be Simple but for other two cases it can land in Simple or Complex. https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderText.cpp (right): https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderText.cpp:1811: { On 2013/12/30 19:29:14, eae wrote: > This method didn't use to have a side effect and now it does, have you ensured > that this is _always_ called now (to set m_textCodePath) and that additional > calls use the new codePath getter instead? This method is always called when RenderText is created and this does not have issue. It just with new changes we are storing the value returned by the CodePath search which we were not. https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderText.h (right): https://codereview.chromium.org/111833006/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderText.h:147: CodePath getCodePath() const { return m_textCodePath;} On 2013/12/30 19:29:14, eae wrote: > Like Eric said, this should be "CodePath codePath() const {" Yes I have fixed this will update code soon with fix.
On 2013/12/30 19:30:30, eae wrote: > Also, please update the change description to better explain what you are > changing and why. > > Finally, if you can please add a unit test for this. Hi, I am not very sure what text case I should add, this patch reduces the call to "CodePath" for text containing only Simple Code Path. Pls help me and suggest what test case I should add.
On 2013/12/31 07:18:48, h.joshi wrote: > On 2013/12/30 19:30:30, eae wrote: > > Also, please update the change description to better explain what you are > > changing and why. > > > > Finally, if you can please add a unit test for this. > > Hi, > I am not very sure what text case I should add, this patch reduces the call to > "CodePath" for text containing only Simple Code Path. > Pls help me and suggest what test case I should add. How about a test that ensure that codePath is only called once for simple text?
On 2014/01/03 17:12:06, eae wrote: > On 2013/12/31 07:18:48, h.joshi wrote: > > On 2013/12/30 19:30:30, eae wrote: > > > Also, please update the change description to better explain what you are > > > changing and why. > > > > > > Finally, if you can please add a unit test for this. > > > > Hi, > > I am not very sure what text case I should add, this patch reduces the call > to > > "CodePath" for text containing only Simple Code Path. > > Pls help me and suggest what test case I should add. > > How about a test that ensure that codePath is only called once for simple text? Okey, will work on creating test case for this.
On 2014/01/07 06:36:00, hj wrote: > On 2014/01/03 17:12:06, eae wrote: > > On 2013/12/31 07:18:48, h.joshi wrote: > > > On 2013/12/30 19:30:30, eae wrote: > > > > Also, please update the change description to better explain what you are > > > > changing and why. > > > > > > > > Finally, if you can please add a unit test for this. > > > > > > Hi, > > > I am not very sure what text case I should add, this patch reduces the call > > to > > > "CodePath" for text containing only Simple Code Path. > > > Pls help me and suggest what test case I should add. > > > > How about a test that ensure that codePath is only called once for simple > text? > > Okey, will work on creating test case for this. Hi, I have added test case for this after going through patch and code some code initially added was not required so removed those changes. Reading with test case are following: Readings With Patch Running 1 tests Running Layout/SimpleTextPathLineLayout.html (1 of 1) RESULT Layout: SimpleTextPathLineLayout: Runs= 51296.0585644 runs/s median= 51270.8567317 runs/s, stdev= 738.942119987 runs/s, min= 49715.3665793 runs/s, max= 52624.816918 runs/s RESULT Layout: SimpleTextPathLineLayout: JSHeap= 1691256.4 bytes median= 1677136.0 bytes, stdev= 21501.462644 bytes, min= 1672592.0 bytes, max= 1722816.0 bytes RESULT Layout: SimpleTextPathLineLayout: Malloc= 0.0 bytes median= 0.0 bytes, stdev= 0.0 bytes, min= 0.0 bytes, max= 0.0 bytes Finished: 20.332721 s Readings Without Patch (Original): Running 1 tests Running Layout/SimpleTextPathLineLayout.html (1 of 1) RESULT Layout: SimpleTextPathLineLayout: Runs= 51690.6550511 runs/s median= 51710.2526816 runs/s, stdev= 380.539110408 runs/s, min= 50803.5964714 runs/s, max= 52143.6700692 runs/s RESULT Layout: SimpleTextPathLineLayout: JSHeap= 1691152.8 bytes median= 1676948.0 bytes, stdev= 21528.0937861 bytes, min= 1672592.0 bytes, max= 1722816.0 bytes RESULT Layout: SimpleTextPathLineLayout: Malloc= 0.0 bytes median= 0.0 bytes, stdev= 0.0 bytes, min= 0.0 bytes, max= 0.0 bytes Finished: 20.586034 s Pls have a look at changes, in case more Simple text is present on Web page then more improvement is observed.
Awesome, thank you. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/111833006/350001
On 2014/01/15 17:02:31, eae wrote: > Awesome, thank you. > > LGTM Thank you
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/111833006/510001
Message was sent while issue was closed.
Change committed as 165203 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
