[debug] Only pass SourceLocation in debug in TurboShaft [v8/v8 : main]
0 views
Skip to first unread message
Leszek Swirski (Gerrit)
Aug 27, 2025, 7:03:24 AM (13 days ago) Aug 27
to Michael Lippautz, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, dmercadi...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Michael Lippautz
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Auto-Submit | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Michael Lippautz
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Aug 27, 2025, 7:10:32 AM (13 days ago) Aug 27
to Leszek Swirski, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, dmercadi...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Leszek Swirski
Michael Lippautz added 1 comment![]( "Open in Gerrit")
File include/cppgc/cross-thread-persistent.h
Line 106, Patchset 1 (Parent):
const SourceLocation& loc = SourceLocation::Current())Michael Lippautz . unresolved
Technically anything that's not in `internal` namespaces is public API and may be used in ways that's breaking. I would not expect it to be a problem except for any virtual functions where it won't compile?
Related details
Attention is currently required from:
- Leszek Swirski
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Aug 27, 2025, 7:19:09 AM (13 days ago) Aug 27
to Michael Lippautz, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, dmercadi...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Michael Lippautz
Leszek Swirski added 1 comment![]( "Open in Gerrit")
File include/cppgc/cross-thread-persistent.h
Line 106, Patchset 1 (Parent):
const SourceLocation& loc = SourceLocation::Current())Michael Lippautz . unresolved
Technically anything that's not in `internal` namespaces is public API and may be used in ways that's breaking. I would not expect it to be a problem except for any virtual functions where it won't compile?
Leszek Swirski
Yeah I think a change from const ref param to value param should only be visible to API users via virtual functions
Related details
Attention is currently required from:
- Michael Lippautz
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Aug 27, 2025, 7:22:53 AM (13 days ago) Aug 27
to Leszek Swirski, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, dmercadi...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Leszek Swirski
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
1 comment
Patchset-level comments
Attention is currently required from:
- Leszek Swirski
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Aug 27, 2025, 7:59:50 AM (13 days ago) Aug 27
to Michael Lippautz, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, dmercadi...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Michael Lippautz
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Auto-Submit | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 1:
Leszek Swirski . resolved
ok there was a virtual function in v8-platform, reverted that one. Needs another stamp
Related details
Attention is currently required from:
- Michael Lippautz
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Aug 27, 2025, 8:08:10 AM (13 days ago) Aug 27
to Leszek Swirski, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, dmercadi...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Leszek Swirski
Michael Lippautz voted![]( "Open in Gerrit")
Attention is currently required from:
- Leszek Swirski
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Aug 27, 2025, 9:07:35 AM (13 days ago) Aug 27
to Michael Lippautz, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, dmercadi...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Michael Lippautz
Leszek Swirski voted and added 2 comments![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Auto-Submit | +1 |
2 comments
Patchset-level comments
File include/cppgc/cross-thread-persistent.h
Line 106, Patchset 1 (Parent):
const SourceLocation& loc = SourceLocation::Current())Michael Lippautz . resolved
Leszek SwirskiTechnically anything that's not in `internal` namespaces is public API and may be used in ways that's breaking. I would not expect it to be a problem except for any virtual functions where it won't compile?
Yeah I think a change from const ref param to value param should only be visible to API users via virtual functions
Leszek Swirski
As you predicted, it was an issue for virtual functions. Fixed.
Related details
Attention is currently required from:
- Michael Lippautz
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Aug 27, 2025, 9:08:33 AM (13 days ago) Aug 27
to Leszek Swirski, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, dmercadi...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Leszek Swirski
Michael Lippautz voted![]( "Open in Gerrit")
Attention is currently required from:
- Leszek Swirski
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
Aug 27, 2025, 9:56:15 AM (13 days ago) Aug 27
to Leszek Swirski, Michael Lippautz, Hannes Payer, cbruni...@chromium.org, dmercadi...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[debug] Only pass SourceLocation in debug in TurboShaft
SourceLocation is almost only used for debug messages in TurboShaft, so
pass it only in debug mode. Additionally, std::source_location (that it
wraps) is a single pointer, so we can efficiently pass it by value
rather than const ref.
This reduces release build d8 by ~170kb, of which ~4kb is the by-value
passing.
Change-Id: Ia6b592f094dad0987b8a886bf16a7d3fe8611eb1
Auto-Submit: Leszek Swirski <les...@chromium.org>
Reviewed-by: Michael Lippautz <mlip...@chromium.org>
Commit-Queue: Leszek Swirski <les...@chromium.org>
Commit-Queue: Michael Lippautz <mlip...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#102072}
Files:
- M include/cppgc/cross-thread-persistent.h
- M include/cppgc/internal/logging.h
- M include/cppgc/internal/pointer-policies.h
- M include/cppgc/persistent.h
- M include/cppgc/platform.h
- M include/cppgc/visitor.h
- M include/v8-platform.h
- M include/v8-source-location.h
- M src/base/logging.h
- M src/codegen/assembler.cc
- M src/codegen/assembler.h
- M src/codegen/code-stub-assembler.cc
- M src/codegen/code-stub-assembler.h
- M src/codegen/macro-assembler.h
- M src/compiler/code-assembler.h
- M src/compiler/turboshaft/assembler.h
- M src/heap/cppgc-js/cpp-heap.cc
- M src/heap/cppgc-js/cpp-snapshot.cc
- M src/heap/cppgc/logging.cc
- M src/heap/cppgc/marking-visitor.cc
- M src/heap/cppgc/marking-visitor.h
- M src/heap/cppgc/platform.cc
- M src/heap/cppgc/platform.h
- M src/objects/casting.h
- M test/unittests/heap/cppgc/persistent-family-unittest.cc
- M test/unittests/heap/cppgc/platform-unittest.cc
Change size: L
Delta: 26 files changed, 147 insertions(+), 160 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages