Push makeExecutable down into AbstractFileWriteAction subclasses
#22609
Conversation
github-actions
bot
added
team-Configurability
| public boolean makeExecutable() { | ||
| // In theory, module maps should not be executable but, in practice, we don't care. As | ||
| // 'executable' is the default (see ActionOutputMetadataStore.setPathReadOnlyAndExecutable()), | ||
| // we want to avoid the extra file operation of making this file non-executable. |
There was a problem hiding this comment.
@justinhorvitz I wonder whether this comment is still correct in light of
It looks like having makeExecutable() return true always results in that file operation, so not sure whether possibly saving another one elsewhere gains anything. I could drop this if you think it's not needed.
There was a problem hiding this comment.
I looked up the history of this comment and found more context. It is referring to a google internal implementation of FileWriteStrategy which only calls setExecutable(false) because our remote execution strategy already makes files executable by default. You can add this clarification to the comment if you wish.
There was a problem hiding this comment.
I see. Could it make sense to have makeExecutable be ternary (with a "don't care" state) to make this more efficient for Bazel?
There was a problem hiding this comment.
It's a good idea but I don't know how to measure the value. I also wonder whether we really need to make anything explicitly not executable, or whether false can be used for "don't care."
There was a problem hiding this comment.
I also don't know how to assess whether this is even worth it. Since ctx.actions.write exposes the executable bit in public API, we would probably need ternary state somewhere. I will consider looking into this as a follow-up at some point.
fmeum
force-pushed
the
abstract-file-write
branch
from
846696c
to
c4b76a9
Compare
fmeum
force-pushed
the
abstract-file-write
branch
4 times, most recently
from
f7dfb8f
to
b32243a
Compare
Getting rid of the single boolean field on `AbstractFileWriteAction` saves 8 bytes on each subclass instance due to padding. Also use a lambda to define `newDeterministicWriter` if possible for improved readability. This prepares for future changes that will add fields to `CppModuleMapAction` to support path mapping. Work towards bazelbuild#6526
fmeum
force-pushed
the
abstract-file-write
branch
from
b32243a
to
545b101
Compare
src/main/java/com/google/devtools/build/lib/analysis/actions/BinaryFileWriteAction.java
Outdated
Show resolved
Hide resolved
| public boolean makeExecutable() { | ||
| // In theory, module maps should not be executable but, in practice, we don't care. As | ||
| // 'executable' is the default (see ActionOutputMetadataStore.setPathReadOnlyAndExecutable()), | ||
| // we want to avoid the extra file operation of making this file non-executable. |
There was a problem hiding this comment.
I looked up the history of this comment and found more context. It is referring to a google internal implementation of FileWriteStrategy which only calls setExecutable(false) because our remote execution strategy already makes files executable by default. You can add this clarification to the comment if you wish.
fmeum
force-pushed
the
abstract-file-write
branch
from
c2d9962
to
a950f83
Compare
|
@justinhorvitz Friendly ping |
github-actions
bot
removed
the
awaiting-review
|
@bazel-io fork 7.3.0 |
Getting rid of the single boolean field on `AbstractFileWriteAction` reduces padding on each subclass instance and in particular frees up a 4-byte field on `CppModuleMapAction`. Also use a lambda to define `newDeterministicWriter` if possible for improved readability. This prepares for future changes that will add fields to `CppModuleMapAction` to support path mapping. Work towards bazelbuild#6526 Closes bazelbuild#22609. PiperOrigin-RevId: 643340715 Change-Id: Id3af26049098e6dfa731f0e7a1be6709bea0d9f2
Getting rid of the single boolean field on `AbstractFileWriteAction` reduces padding on each subclass instance and in particular frees up a 4-byte field on `CppModuleMapAction`. Also use a lambda to define `newDeterministicWriter` if possible for improved readability. This prepares for future changes that will add fields to `CppModuleMapAction` to support path mapping. Work towards bazelbuild#6526 Closes bazelbuild#22609. PiperOrigin-RevId: 643340715 Change-Id: Id3af26049098e6dfa731f0e7a1be6709bea0d9f2
…classes (#22845) Getting rid of the single boolean field on `AbstractFileWriteAction` reduces padding on each subclass instance and in particular frees up a 4-byte field on `CppModuleMapAction`. Also use a lambda to define `newDeterministicWriter` if possible for improved readability. This prepares for future changes that will add fields to `CppModuleMapAction` to support path mapping. Work towards #6526 Closes #22609. PiperOrigin-RevId: 643340715 Change-Id: Id3af26049098e6dfa731f0e7a1be6709bea0d9f2 Closes #22749 Closes #22750
Getting rid of the single boolean field on
AbstractFileWriteActionreduces padding on each subclass instance and in particular frees up a 4-byte field onCppModuleMapAction.Also use a lambda to define
newDeterministicWriterif possible for improved readability.This prepares for future changes that will add fields to
CppModuleMapActionto support path mapping.Work towards #6526