Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(167)

Issue 22867032: Use Finch to compare the performances of CLD1 and CLD2 (Closed)

Created:
7 years, 4 months ago by hajimehoshi
Modified:
7 years, 3 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Suppression the problems on the build bots: There were two reasons why that CL broke the build tree. One is the size bloating, which was solved by https://codereview.chromium.org/23460018. The other is compiling error on ARM, and this was also solved by updating CLD2 (https://chromiumcodereview.appspot.com/23606017/). -- Use Finch to compare the performances of CLD1 and CLD2 Add a compile time constant CLD_VERSION, which indicates the version of CLD. If this is not define, Finch test to compare CLD1 and CLD2 is supposed to be used. By this CL, each platform will have the below status: Linux: Use both CLD1 and CLD2 (and use Finch). Mac OS X: Use both CLD1 and CLD2 (and use Finch). Windows: Use only CLD1 once because now CLD2 can't be compiled on Windows. After we can have CLD2 compiled on Windows, we will use CLD2 and Finch asap. iOS: Still use only CLD1. (It's because it is hard to use both CLD1 and CLD2 on mobile platform because of the binary size impact.) Android: Still use only CLD1. (The same reason as iOS) So some platforms will have two CLD binaries, but this is temporal in the sense that we intend to use Finch only for Dev and Beta channel. Before releasing the stable Chromium version, we decide which version of CLD is adopted, make another CL to use only one CLD, and send a merge request. (Of course, we hope we will be able to adopt CLD2.) BUG=240647 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221380 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221675

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : Bug fix: remove dependencies to the files of CLD2 which include 'main' functions #

Patch Set 4 : Bug fix: add '%' #

Patch Set 5 : (rebasing) #

Patch Set 6 : Bug fix: Android's 'check_webview_licenses' tries to check all third_party libraries' licenses, but… #

Patch Set 7 : (rebasing) #

Patch Set 8 : Stop using CLD2 on Windows once #

Total comments: 4

Patch Set 9 : nits: Modify README.chromium #

Patch Set 10 : (rebasing) #

Patch Set 11 : (rebasing) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+225 lines, -22 lines) Patch
M build/all.gyp View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M build/common.gypi View 1 2 3 4 5 6 7 8 9 10 5 chunks +16 lines, -0 lines 0 comments Download
M chrome/chrome_common.gypi View 1 2 3 4 5 6 2 chunks +10 lines, -1 line 0 comments Download
M chrome/chrome_dll.gypi View 1 2 chunks +10 lines, -1 line 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 2 chunks +0 lines, -2 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 8 9 10 4 chunks +16 lines, -10 lines 0 comments Download
M chrome/common/DEPS View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/translate/language_detection_util.cc View 1 4 chunks +92 lines, -8 lines 0 comments Download
M third_party/cld/README.chromium View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
A + third_party/cld_2/LICENSE View 1 2 3 4 5 0 chunks +-1 lines, --1 lines 0 comments Download
A + third_party/cld_2/OWNERS View 1 0 chunks +-1 lines, --1 lines 0 comments Download
A third_party/cld_2/README.chromium View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -0 lines 0 comments Download
A third_party/cld_2/cld_2.gyp View 1 2 1 chunk +71 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
hajimehoshi
Can you take a look if you have time?
7 years, 4 months ago (2013-08-22 11:43:31 UTC) #1
Takashi Toyoshima
It's bad to have a condition no trybot cover it. Can you remove the case ...
7 years, 3 months ago (2013-09-03 02:20:22 UTC) #2
hajimehoshi
+sky@, can you take a look at gypi files and DEPS file? toyoshim@, can you ...
7 years, 3 months ago (2013-09-04 04:56:38 UTC) #3
sky
What's the impact on the binary size of including both? I assume you're not letting ...
7 years, 3 months ago (2013-09-04 14:35:53 UTC) #4
hajimehoshi
Right. We need to choose only one CLD for the stable channel. Here are the ...
7 years, 3 months ago (2013-09-04 15:28:13 UTC) #5
hajimehoshi
BTW, we've already discussed with laforge@ and decided to adopt this plan. Sorry for the ...
7 years, 3 months ago (2013-09-04 15:50:04 UTC) #6
sky
LGTM
7 years, 3 months ago (2013-09-04 15:57:12 UTC) #7
Takashi Toyoshima
LGTM with nits. Also please runs src/tools/licenses.py to check if your license description is in ...
7 years, 3 months ago (2013-09-05 06:33:39 UTC) #8
hajimehoshi
Thank you! https://codereview.chromium.org/22867032/diff/48001/third_party/cld_2/README.chromium File third_party/cld_2/README.chromium (right): https://codereview.chromium.org/22867032/diff/48001/third_party/cld_2/README.chromium#newcode4 third_party/cld_2/README.chromium:4: Version: unknown On 2013/09/05 06:33:39, Takashi Toyoshima ...
7 years, 3 months ago (2013-09-05 06:52:10 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/22867032/65001
7 years, 3 months ago (2013-09-05 06:57:17 UTC) #10
commit-bot: I haz the power
Retried try job too often on ios_dbg_simulator for step(s) ui_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_simulator&number=83107
7 years, 3 months ago (2013-09-05 08:06:17 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/22867032/65001
7 years, 3 months ago (2013-09-05 08:23:09 UTC) #12
commit-bot: I haz the power
Change committed as 221380
7 years, 3 months ago (2013-09-05 11:17:04 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/22867032/85001
7 years, 3 months ago (2013-09-06 07:51:44 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/22867032/85001
7 years, 3 months ago (2013-09-06 09:47:10 UTC) #15
commit-bot: I haz the power
7 years, 3 months ago (2013-09-06 11:03:08 UTC) #16
Message was sent while issue was closed.
Change committed as 221675

Powered by Google App Engine
This is Rietveld 408576698