|
|
Created:
5 years, 6 months ago by chcunningham Modified:
5 years, 6 months ago Reviewers:
Dmitry Lomov (no reviews), calamity, tdanderson, Mike West, DaleCurtis, Rune Fevang, Nicolas Zea CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate perf expectations since static linking ffmpeg.
This updates bss sizes for the following:
- linux-release-64/sizes/chrome-bss/bss
- linux-release/sizes/chrome-bss/bss
- mac-release/sizes/ChromiumFramework/ChromiumFramework
- xp-release/sizes/chrome_child.dll/chrome_child.dll
It also updates linux-release chrome-textrel count to take on the textrelocations that were previously part of ffmpegsumo.so.
Since the introduction of textrel expectations (https://codereview.chromium.org/9187031), ffmpegsumo has been known to have textrelocations that are not trivial to remove. The good news is, the number of relocations has been steadily falling as we roll from upstream ffmpeg.
While chrome/chrome_child have grown significantly, the net change
(now that ffmpegsumo.dll is gone) is actually a savings of a few
hundred KB due to dead code elimination in static linking.
Static linking change:
https://codereview.chromium.org/1141703002/
Bugs motivating the original static linking change:
BUG=435455, 429131, 441908
Committed: https://crrev.com/f37609eb69066f9531a4cce126e8225819217250
Cr-Commit-Position: refs/heads/master@{#333462}
Patch Set 1 #Patch Set 2 : Choosing new range for re-landed ffmpeg change #Patch Set 3 : Updating chrome textrel and broadening range. #Messages
Total messages: 30 (9 generated)
chcunningham@chromium.org changed reviewers: + calamity@chromium.org, dalecurtis@chromium.org, mkwst@chromium.org, zea@chromium.org
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1161633003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1161633003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/41c1b31dc7983a1dca627f9e00ee3977b75e5b6f Cr-Commit-Position: refs/heads/master@{#332938}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1157993006/ by thestig@chromium.org. The reason for reverting is: r332994 reverted the static linking ffmpeg CL and the size expectations is now red due to "improvement"..
Message was sent while issue was closed.
On 2015/06/05 03:04:44, Lei Zhang wrote: > A revert of this CL (patchset #1 id:1) has been created in > https://codereview.chromium.org/1157993006/ by mailto:thestig@chromium.org. > > The reason for reverting is: r332994 reverted the static linking ffmpeg CL and > the size expectations is now red due to "improvement".. Thanks for doing this Lei!
FYI, the related ffmpeg change is relanded. I'm redoing the sizes change now as build will soon fail ;) Expect new patchset shortly
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org Link to the patchset: https://codereview.chromium.org/1161633003/#ps20001 (title: "Choosing new range for re-landed ffmpeg change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1161633003/20001
Heads up. The new range chosen is pretty small - there just aren't a lot of commits coming in atm. I'll broaden it further tomorrow if needed.
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/62cc1d7aefd10962b5b7ca4be579cf561e4d5f68 Cr-Commit-Position: refs/heads/master@{#333436}
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org Link to the patchset: https://codereview.chromium.org/1161633003/#ps40001 (title: "Updating chrome textrel and broadening range.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1161633003/40001
chcunningham@chromium.org changed reviewers: + dslomov@chromium.org, rfevang@chromium.org, tdanderson@chromium.org
All, Please think of this as TBR. I don't yet have committer status so I can't actually do a TBR submit - but I think this needs to go in now as the sizes check will continue to fail and cause confusion.
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f37609eb69066f9531a4cce126e8225819217250 Cr-Commit-Position: refs/heads/master@{#333462}
Message was sent while issue was closed.
On 2015/06/09 08:06:19, commit-bot: I haz the power wrote: > Patchset 3 (id:??) landed as > https://crrev.com/f37609eb69066f9531a4cce126e8225819217250 > Cr-Commit-Position: refs/heads/master@{#333462} In a follow up CL, maybe it would be better to reduce the text-rel regression number to be = ffmpeg_textrel_count + 1 . This will preserve the enforcement that chrome should *never* add textrels, while possibly adding a little pain when we roll ffmpeg from upstream (unlikely to be an issue, the numbers seem to consistently fall). As is, the textrel regression nubmer is auto generated from make_expectations, which allows for small increases from the ffmpeg baseline. WDYT?
Message was sent while issue was closed.
Probably we don't want to tie ffmpeg into the build tools like that. We should make sure to run the sizes tool ahead of time with the next roll so these can updated at the same time. Can you add some steps to the ffmpeg roll documentation about that?
Message was sent while issue was closed.
On 2015/06/09 16:08:22, DaleCurtis wrote: > Probably we don't want to tie ffmpeg into the build tools like that. We should > make sure to run the sizes tool ahead of time with the next roll so these can > updated at the same time. Can you add some steps to the ffmpeg roll > documentation about that? OK, I will update the doc. Also, I checked the perf dashboard cold start tests for linux release and found that nothing was regressed. |