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

Issue 918303005: Avoid repeating code when creating builtins. (Closed)

Created:
5 years, 10 months ago by Daniel Bratell
Modified:
5 years, 9 months ago
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

Avoid repeating code when creating builtins. InstallBuiltinFunctionIds stood out when looking at code that grew strangely when compiled with default optimizations. This change from repeated code to a loop saves 6-7 KB of machine code. I suspect it's faster but I also suspect it's fast enough either way so that is not really a factor. Machine code reduction seen below. Code formatted with git cl format. clang x64: Total change: -5985 bytes ------------------------------------------- +517 - Source: ?? - (gained 744, lost 227) ------------------------------------------- New symbols: +744: v8::internal::Genesis::InstallBuiltinFunctionIds()::builtins type=d, size=744 bytes Removed symbols: -4: .L.str98 type=r, size=4 bytes ... [stripped 30 similar lines] -19: .L.str100 type=r, size=19 bytes ---------------------------------------------------------------------------------------------- -6502 - Source: /home/bratell/src/chromium/src/v8/src/bootstrapper.cc - (gained 0, lost 6502) ---------------------------------------------------------------------------------------------- Removed symbols: -1135: v8::internal::ResolveBuiltinIdHolder(v8::internal::Handle<v8::internal::Context>, char const*) type=t, size=1135 bytes Shrunk symbols: -5367: v8::internal::Genesis::InstallBuiltinFunctionIds() type=t, (was 7105 bytes, now 1738 bytes) BUG= Committed: https://crrev.com/bd21d72d507625e5caf70f31ee11c8c21ead1651 Cr-Commit-Position: refs/heads/master@{#26919}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -7 lines) Patch
M src/bootstrapper.cc View 1 chunk +16 lines, -7 lines 0 comments Download

Messages

Total messages: 7 (2 generated)
Daniel Bratell
dslomov, can you please take a look. Not high priority, just random cleanup. And git ...
5 years, 10 months ago (2015-02-19 15:39:47 UTC) #2
Dmitry Lomov (no reviews)
lgtm, sorry for delay
5 years, 10 months ago (2015-02-26 14:20:12 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/918303005/1
5 years, 9 months ago (2015-02-27 14:50:20 UTC) #5
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 9 months ago (2015-02-27 15:21:47 UTC) #6
commit-bot: I haz the power
5 years, 9 months ago (2015-02-27 15:22:00 UTC) #7
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/bd21d72d507625e5caf70f31ee11c8c21ead1651
Cr-Commit-Position: refs/heads/master@{#26919}

Powered by Google App Engine
This is Rietveld 408576698