Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(76)

Issue 2559063002: Don't define log2 and log2f in blink (Closed)

Created:
2 years, 7 months ago by brucedawson
Modified:
2 years, 7 months ago
Reviewers:
Nico
CC:
chromium-reviews, blink-reviews, blink-reviews-wtf_chromium.org, Mikhail
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't define log2 and log2f in blink The consequences are surprising. A long time ago VC++'s CRT didn't supply log2 and log2f so WebKit/blink supplied them as inline functions in MathExtras.h. In some cases (including the full official builds that we ship to customers) this meant that when Skia called log2f they ended up pulling in blink's version. This pulled in PeriodicWave.obj which then pulled in various other object files, and somehow the unreferenced-code logic in the linker was unable to get rid of all of the rubbish. I noticed this because during the gyp->gn transition I noticed that ff_cos_131072 and friends were in gn builds of chrome.dll but not gyp builds. I fixed this but then they came back in full LTCG official builds (full_wpo_on_official = true). Analysis of a verbose link gave this chain of events for why rdft.obj (the definer of ff_*) was pulled in: rdft.obj referenced by avfft.obj for symbol ff_rdft_init avfft.obj referenced by FFTFrameFFMPEG.obj for symbol av_rdft_calc FFTFrameFFMPEG.obj referenced by PeriodicWave.obj for symbol public: void __thiscall blink::FFTFrame::doInverseFFT(float *) PeriodicWave.obj referenced by SkGeometry.obj for symbol log2f Usually the trick to resolving these unwanted symbols is changing a source_set to static_library, but this time it was recognizing that SkGeometry has no business pulling in PeriodicWave, and removing the VC++ definition of log2f that is the culprit. The effect on the size of chrome.dll is: chrome.dll .text: -6656 bytes change .rdata: 240 bytes change .data: -786624 bytes change .rodata: -592 bytes change .reloc: -872 bytes change Total change: -794504 bytes BUG=630755 Committed: https://crrev.com/036e2ceb51ebaed01c6ad552fffc227be20c655e Cr-Commit-Position: refs/heads/master@{#437181}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -2 lines) Patch
M third_party/WebKit/Source/wtf/MathExtras.h View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 15 (10 generated)
brucedawson
PTAL I'm going to run tests to see if these can be removed for Android ...
2 years, 7 months ago (2016-12-08 03:57:32 UTC) #7
Nico
On 2016/12/08 03:57:32, brucedawson wrote: > PTAL > > I'm going to run tests to ...
2 years, 7 months ago (2016-12-08 04:14:15 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2559063002/1
2 years, 7 months ago (2016-12-08 04:45:50 UTC) #10
commit-bot: I haz the power
Committed patchset #1 (id:1)
2 years, 7 months ago (2016-12-08 04:50:11 UTC) #13
commit-bot: I haz the power
2 years, 7 months ago (2016-12-08 04:52:45 UTC) #15
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/036e2ceb51ebaed01c6ad552fffc227be20c655e
Cr-Commit-Position: refs/heads/master@{#437181}

Powered by Google App Engine
This is Rietveld 408576698