Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243433002:
Switching from the old language detector (CLD2) to the new one (CLD3) (Closed)
|
Created:
4 years, 4 months ago by Anton Bakalov Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitching from the old language detector (CLD2) to the new one (CLD3)
BUG=624904
Committed: https://crrev.com/c2d06cc46ee69d6a90e9374c5b25ab4e4eaabe4f
Cr-Commit-Position: refs/heads/master@{#411377}
Patch Set 1 #
Messages
Total messages: 22 (9 generated)
The CQ bit was checked by abakalov@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...
Description was changed from ========== Switching from the old language detector (CLD2) to the new one (CLD3) BUG= ========== to ========== Switching from the old language detector (CLD2) to the new one (CLD3) BUG=624904 ==========
abakalov@chromium.org changed reviewers: + andrewhayden@chromium.org, brettw@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM! Let's do it!
On 2016/08/11 17:01:05, Andrew Hayden (chromium.org) wrote: > LGTM! Let's do it! Thanks, Andrew!
The CQ bit was checked by abakalov@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/11 18:06:28, brettw (ping on IM after 24h) wrote: > lgtm Thanks, Brett!
Message was sent while issue was closed.
Description was changed from ========== Switching from the old language detector (CLD2) to the new one (CLD3) BUG=624904 ========== to ========== Switching from the old language detector (CLD2) to the new one (CLD3) BUG=624904 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Switching from the old language detector (CLD2) to the new one (CLD3) BUG=624904 ========== to ========== Switching from the old language detector (CLD2) to the new one (CLD3) BUG=624904 Committed: https://crrev.com/c2d06cc46ee69d6a90e9374c5b25ab4e4eaabe4f Cr-Commit-Position: refs/heads/master@{#411377} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c2d06cc46ee69d6a90e9374c5b25ab4e4eaabe4f Cr-Commit-Position: refs/heads/master@{#411377}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2237913003/ by xidachen@chromium.org. The reason for reverting is: Causing build failure: https://build.chromium.org/p/chromium/builders/Mac/builds/18465 Error message: # language_identifier_features.cc _GLOBAL__sub_I_language_identifier_features.cc+0x9a # language_identifier_features.cc chrome_lang_id::RegistryMetadata::Register(chrome_lang_id::RegistryMetadata*) # language_identifier_features.cc _ZN14chrome_lang_id17RegisterableClassINS_15FeatureFunctionINS_8SentenceEJEEEE9registry_E # language_identifier_features.cc chrome_lang_id::__ContinuousBagOfNgramsFunction__factory() # language_identifier_features.cc chrome_lang_id::__ContinuousBagOfNgramsFunction__registrar # language_identifier_features.cc chrome_lang_id::__ContinuousBagOfNgramsFunction__registrar+0x10 # language_identifier_features.cc chrome_lang_id::__ContinuousBagOfNgramsFunction__registrar+0x18 # language_identifier_features.cc chrome_lang_id::__ContinuousBagOfNgramsFunction__registrar+0x20 # language_identifier_features.cc chrome_lang_id::__ContinuousBagOfNgramsFunction__registrar+0x28 # language_identifier_features.cc operator new[](unsigned long) # language_identifier_features.cc sqlite3VXPrintf.zOrd+0x1b67aa # language_identifier_features.cc sqlite3VXPrintf.zOrd+0x1b67c3 # language_identifier_features.cc sqlite3VXPrintf.zOrd+0x1b67e1.
Message was sent while issue was closed.
Other relevant bits from the log:
# Static initializers in /b/c/b/mac_ng/src/out/Release/Chromium
Framework.framework/Chromium Framework:
# HINT: To get this list, run tools/mac/show_mod_init_func.py
# HINT: diff against the log from the last run to see what changed
/b/c/b/mac_ng/src/out/Release/Chromium Framework.unstripped
0x0000000004e95b80 @ _GLOBAL__sub_I_language_identifier_features.cc (in Chromium
Framework.unstripped) (language_identifier_features.cc:0)
RESULT Chromium: Chromium= 9284 bytes
RESULT Chromium-__TEXT: __TEXT= 4096 bytes
RESULT Chromium-__DATA: __DATA= 4096 bytes
RESULT Chromium-__OBJC: __OBJC= 0 bytes
RESULT ChromiumFramework: ChromiumFramework= 120123408 bytes
RESULT ChromiumFramework-__TEXT: __TEXT= 113786880 bytes
RESULT ChromiumFramework-__DATA: __DATA= 6250496 bytes
RESULT ChromiumFramework-__OBJC: __OBJC= 0 bytes
RESULT Chromium.app: Chromium.app= 175259648 bytes
RESULT chrome-si: initializers= 2 files
<Thread(Thread-1, started 4546961408)> ProcessRead: proc.stdout finished.
<Thread(Thread-1, started 4546961408)> ProcessRead: cleaning up.
<Thread(Thread-2, started daemon 4551168000)> TimedFlush: Finished
<Thread(Thread-1, started 4546961408)> ProcessRead: finished.
Set PYTHONPATH:
/b/rr/tmpnk4aKn/rw/checkout/scripts:/b/rr/tmpnk4aKn/rw/checkout/site_config:/b/rr/tmpnk4aKn/rw/checkout/third_party:/b/rr/tmpnk4aKn/rw/checkout/third_party/buildbot_8_4p1:/b/rr/tmpnk4aKn/rw/checkout/third_party/buildbot_slave_8_4:/b/rr/tmpnk4aKn/rw/checkout/third_party/coverage-3.7.1:/b/rr/tmpnk4aKn/rw/checkout/third_party/decorator_3_3_1:/b/rr/tmpnk4aKn/rw/checkout/third_party/google_api_python_client:/b/rr/tmpnk4aKn/rw/checkout/third_party/httplib2/python2:/b/rr/tmpnk4aKn/rw/checkout/third_party/infra_libs:/b/rr/tmpnk4aKn/rw/checkout/third_party/jinja2:/b/rr/tmpnk4aKn/rw/checkout/third_party/markupsafe:/b/rr/tmpnk4aKn/rw/checkout/third_party/mock-1.0.1:/b/rr/tmpnk4aKn/rw/checkout/third_party/oauth2client:/b/rr/tmpnk4aKn/rw/checkout/third_party/requests_2_10_0:/b/rr/tmpnk4aKn/rw/checkout/third_party/setuptools-0.6c11:/b/rr/tmpnk4aKn/rw/checkout/third_party/sqlalchemy_0_7_1:/b/rr/tmpnk4aKn/rw/checkout/third_party/sqlalchemy_migrate_0_7_1:/b/rr/tmpnk4aKn/rw/checkout/third_party/tempita_0_5:/b/rr/tmpnk4aKn/rw/checkout/third_party/twisted_10_2:/b/rr/tmpnk4aKn/rw/checkout/third_party/uritemplate:/b/rr/tmpnk4aKn/rw/checkout/third_party/site-packages:/b/rr/tmpnk4aKn/rw/checkout/scripts/tools:/b/build/site_config:/b/build/scripts:/b/build/scripts/release:/b/build/third_party:/b/build/third_party/requests_2_10_0:/b/build_internal/site_config:/b/build_internal/symsrc:/b/build/slave:/b/build/third_party/buildbot_slave_8_4:/b/build/third_party/twisted_10_2:/b/c/b/mac_ng:/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python27.zip:/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7:/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-darwin:/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-mac:/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-mac/lib-scriptpackages:/System/Library/Frameworks/Python.framework/Versions/2.7/Extras/lib/python:/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/lib-tk:/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/lib-old:/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/lib-dynload:/System/Library/Frameworks/Python.framework/Versions/2.7/Extras/lib/python/PyObjC
Command ['/usr/bin/python',
u'/b/rr/tmpnk4aKn/rw/checkout/scripts/tools/runit.py', '--show-path',
'/usr/bin/python', u'/b/rr/tmpnk4aKn/rw/checkout/scripts/slave/runtest.py',
'--target', 'Release', '--xvfb', '--builder-name', u'Mac', '--slave-name',
u'vm682-m1', '--build-number', '18465', '--build-properties', '{"buildnumber":
18465, "slavename": "vm682-m1", "target_platform": "mac", "mastername":
"chromium", "buildername": "Mac"}', '--test-type', 'sizes',
'--run-python-script',
'/b/c/b/mac_ng/src/infra/scripts/legacy/scripts/slave/chromium/sizes.py',
'--json', '/tmp/tmpcvJUiw'] returned exit code 0
FAILED mac-release/sizes/chrome-si/initializers: actual 2, expected 0, better
lower
Question, since when do we require that third-party code be free of static
initializers? I know we have this constraint in Chromium itself, but I wasn't
aware that third-party code was subject to the same restrictions. Also, there's
a sheriff item here for why all the bots were green with this CL - the static
initializer check that is running here should have stopped submit.
Anton, the fix is presumably to remove the static initializers in CLD3 and
replace them with any alternative.
Message was sent while issue was closed.
> Question, since when do we require that third-party code be free of static > initializers? I know we have this constraint in Chromium itself, but I wasn't > aware that third-party code was subject to the same restrictions. We want to have no static initializers. Currently the test reports there are 0, and significant work has been done so that this is the case across all of the code that Chrome uses. Plus, this isn't exactly third_party code. It's written at Google. Just because you check your code into the third_party directory doesn't mean that no rules apply to you. This check is unfortunately implemented as a performance test which only runs on the perf waterfall. It would be nice if this weren't the case, but static initializers don't get added so often that this is huge issue as far as I've seen.
Message was sent while issue was closed.
On 2016/08/11 22:34:55, brettw (ping on IM after 24h) wrote: > > Question, since when do we require that third-party code be free of static > > initializers? I know we have this constraint in Chromium itself, but I wasn't > > aware that third-party code was subject to the same restrictions. > > We want to have no static initializers. Currently the test reports there are 0, > and significant work has been done so that this is the case across all of the > code that Chrome uses. Plus, this isn't exactly third_party code. It's written > at Google. Just because you check your code into the third_party directory > doesn't mean that no rules apply to you. > > This check is unfortunately implemented as a performance test which only runs on > the perf waterfall. It would be nice if this weren't the case, but static > initializers don't get added so often that this is huge issue as far as I've > seen. Sorry about this issue! We were relying on a component registration mechanism written a few years ago. I am not sure how it got submitted even though the style guide does not allow it.
Message was sent while issue was closed.
Thanks for the context, Brett. I've filed crbug.com/637202 to follow up on trying to stop this from happening in the future to others. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
