8000 Always capture mutable GlobalState with as const by jez · Pull Request #8792 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Always capture mutable GlobalState with as const #8792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 24, 2025
Merged

Always capture mutable GlobalState with as const #8792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
298a03f
Always capture mutable GlobalState with as const
jez Apr 24, 2025
b428f73
clang-format
jez Apr 24, 2025
6e422e3
ast/substitute doesn't actually need a MutableContext
jez Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions ast/substitute/Substitute.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ template <typename T> class SubstWalk {
private:
T &subst;

void substClassName(core::MutableContext ctx, ExpressionPtr &node) {
void substClassName(core::Context ctx, ExpressionPtr &node) {
ExpressionPtr *cur = &node;
while (true) {
auto constLit = cast_tree<UnresolvedConstantLit>(*cur);
Expand All @@ -28,7 +28,7 @@ template <typename T> class SubstWalk {
}
}

void substArg(core::MutableContext ctx, ExpressionPtr &argp) {
void substArg(ExpressionPtr &argp) {
ExpressionPtr *arg = &argp;
while (arg != nullptr) {
typecase(
Expand All @@ -55,30 +55,30 @@ template <typename T> class SubstWalk {
public:
SubstWalk(T &subst) : subst(subst) {}

void preTransformClassDef(core::MutableContext ctx, ExpressionPtr &tree) {
void preTransformClassDef(core::Context ctx, ExpressionPtr &tree) {
auto &original = cast_tree_nonnull<ClassDef>(tree);
substClassName(ctx, original.name);
for (auto &anc : original.ancestors) {
substClassName(ctx, anc);
}
}

void preTransformMethodDef(core::MutableContext ctx, ExpressionPtr &tree) {
void preTransformMethodDef(core::Context ctx, ExpressionPtr &tree) {
auto &original = cast_tree_nonnull<MethodDef>(tree);
original.name = subst.substituteSymbolName(original.name);
for (auto &arg : original.args) {
substArg(ctx, arg);
substArg(arg);
}
}

void preTransformBlock(core::MutableContext ctx, ExpressionPtr &tree) {
void preTransformBlock(core::Context ctx, ExpressionPtr &tree) {
auto &original = cast_tree_nonnull<Block>(tree);
for (auto &arg : original.args) {
substArg(ctx, arg);
substArg(arg);
}
}

void postTransformUnresolvedIdent(core::MutableContext ctx, ExpressionPtr &original) {
void postTransformUnresolvedIdent(core::Context ctx, ExpressionPtr &original) {
auto &id = cast_tree_nonnull<UnresolvedIdent>(original);
if (id.kind != ast::UnresolvedIdent::Kind::Local) {
id.name = subst.substituteSymbolName(cast_tree<UnresolvedIdent>(original)->name);
Expand All @@ -87,12 +87,12 @@ template <typename T> class SubstWalk {
}
}

void postTransformLocal(core::MutableContext ctx, ExpressionPtr &local) {
void postTransformLocal(core::Context ctx, ExpressionPtr &local) {
cast_tree_nonnull<Local>(local).localVariable._name =
subst.substitute(cast_tree_nonnull<Local>(local).localVariable._name);
}

void preTransformSend(core::MutableContext ctx, ExpressionPtr &original) {
void preTransformSend(core::Context ctx, ExpressionPtr &original) {
auto &send = cast_tree_nonnull<Send>(original);
if (send.fun == core::Names::aliasMethod()) {
// `alias_method :foo :bar` is very similar to `def foo; self.bar(); end`, so in
Expand Down Expand Up @@ -136,7 +136,7 @@ template <typename T> class SubstWalk {
}
}

void postTransformLiteral(core::MutableContext ctx, ExpressionPtr &tree) {
void postTransformLiteral(core::Context ctx, ExpressionPtr &tree) {
auto &original = cast_tree_nonnull<Literal>(tree);
auto nameRef = unwrapLiteralToName(tree);
if (!nameRef.exists()) {
Expand All @@ -157,26 +157,26 @@ template <typename T> class SubstWalk {
}
}

void postTransformUnresolvedConstantLit(core::MutableContext ctx, ExpressionPtr &tree) {
void postTransformUnresolvedConstantLit(core::Context ctx, ExpressionPtr &tree) {
auto &original = cast_tree_nonnull<UnresolvedConstantLit>(tree);
original.cnst = subst.substituteSymbolName(original.cnst);
substClassName(ctx, original.scope);
}

void postTransformRuntimeMethodDefinition(core::MutableContext ctx, ExpressionPtr &original) {
void postTransformRuntimeMethodDefinition(core::Context ctx, ExpressionPtr &original) {
auto &def = cast_tree_nonnull<RuntimeMethodDefinition>(original);
def.name = subst.substitute(def.name);
}
};
} // namespace

ParsedFile Substitute::run(core::MutableContext ctx, const core::NameSubstitution &subst, ParsedFile what) {
ParsedFile Substitute::run(core::Context ctx, const core::NameSubstitution &subst, ParsedFile what) {
SubstWalk walk(subst);
TreeWalk::apply(ctx, walk, what.tree);
return what;
}

ParsedFile Substitute::run(core::MutableContext ctx, core::LazyNameSubstitution &subst, ParsedFile what) {
ParsedFile Substitute::run(core::Context ctx, core::LazyNameSubstitution &subst, ParsedFile what) {
SubstWalk walk(subst);
TreeWalk::apply(ctx, walk, what.tree);
return what;
Expand Down
4 changes: 2 additions & 2 deletions ast/substitute/substitute.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ class LazyNameSubstitution;
namespace sorbet::ast {
class Substitute {
public:
static ParsedFile run(core::MutableContext ctx, const core::NameSubstitution &subst, ParsedFile what);
static ParsedFile run(core::MutableContext ctx, core::LazyNameSubstitution &subst, ParsedFile what);
static ParsedFile run(core::Context ctx, const core::NameSubstitution &subst, ParsedFile what);
static ParsedFile run(core::Context ctx, core::LazyNameSubstitution &subst, ParsedFile what);
};
} // namespace sorbet::ast
#endif // SORBET_SUBSTITUTE_H
54 changes: 28 additions & 26 deletions main/pipeline/pipeline.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,39 +578,41 @@ ast::ParsedFilesOrCancelled mergeIndexResults(core::GlobalState &cgs, const opti
Timer timeit(cgs.tracer(), "substituteTrees");
auto resultq = make_shared<BlockingBoundedQueue<vector<ast::ParsedFile>>>(batchq->bound);

workers.multiplexJob("substituteTrees", [&cgs, &logger = cgs.tracer(), batchq, resultq, cancelable]() {
Timer timeit(logger, "substituteTreesWorker");
IndexSubstitutionJob job;
int numTreesProcessed = 0;
std::vector<ast::ParsedFile> trees;
for (auto result = batchq->try_pop(job); !result.done(); result = batchq->try_pop(job)) {
if (result.gotItem()) {
// Unconditionally update the total to avoid starving the consumer thread
numTreesProcessed += job.numTreesProcessed;
workers.multiplexJob(
"substituteTrees", [&cgs = as_const(cgs), &logger = cgs.tracer(), batchq, resultq, cancelable]() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am on my phone, so it's hard to be 100% sure, but it looks like we don't mutate GlobalState in this function, so we could just take it as a const in this function from the start?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the earlier block is using cgs mutably (I think the earlier block is where we actually merge the GlobalStates.

That's probably worth another look just to be sure, but I think we can't change mergeIndexResults to take a const GlobalState &

Timer timeit(logger, "substituteTreesWorker");
IndexSubstitutionJob job;
int numTreesProcessed = 0;
std::vector<ast::ParsedFile> trees;
for (auto result = batchq->try_pop(job); !result.done(); result = batchq->try_pop(job)) {
if (result.gotItem()) {
// Unconditionally update the total to avoid starving the consumer thread
numTreesProcessed += job.numTreesProcessed;

// If the slow path has been cancelled, skip substitution to handle the tree dropping in once place.
if (cancelable && cgs.epochManager->wasTypecheckingCanceled()) {
continue;
}
// If the slow path has been cancelled, skip substitution to handle the tree dropping in once
// place.
if (cancelable && cgs.epochManager->wasTypecheckingCanceled()) {
continue;
}

if (job.subst.has_value()) {
for (auto &tree : job.trees) {
if (!tree.cached()) {
core::MutableContext ctx(cgs, core::Symbols::root(), tree.file);
tree = ast::Substitute::run(ctx, *job.subst, move(tree));
if (job.subst.has_value()) {
for (auto &tree : job.trees) {
if (!tree.cached()) {
core::Context ctx(cgs, core::Symbols::root(), tree.file);
tree = ast::Substitute::run(ctx, *job.subst, move(tree));
}
}
}
}

trees.insert(trees.end(), std::make_move_iterator(job.trees.begin()),
std::make_move_iterator(job.trees.end()));
trees.insert(trees.end(), std::make_move_iterator(job.trees.begin()),
std::make_move_iterator(job.trees.end()));
}
}
}

if (numTreesProcessed > 0) {
resultq->push(std::move(trees), numTreesProcessed);
}
});
if (numTreesProcessed > 0) {
resultq->push(std::move(trees), numTreesProcessed);
}
});

ret.reserve(totalNumTrees);
vector<ast::ParsedFile> trees;
Expand Down
2 changes: 2 additions & 0 deletions packager/VisibilityChecker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include <algorithm>
#include <iterator>

using namespace std;

using namespace std::literals::string_view_literals;

namespace sorbet::packager {
Expand Down
2 changes: 1 addition & 1 deletion packager/packager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2096,7 +2096,7 @@ void packageRunCore(core::GlobalState &gs, WorkerPool &workers, absl::Span<ast::
}
}

workers.multiplexJob("rewritePackagesAndFiles", [&gs, &files, &barrier, taskq]() {
workers.multiplexJob("rewritePackagesAndFiles", [&gs = as_const(gs), &files, &barrier, taskq]() {
Timer timeit(gs.tracer(), "packager.rewritePackagesAndFilesWorker");
size_t idx;
for (auto result = taskq->try_pop(idx); !result.done(); result = taskq->try_pop(idx)) {
Expand Down
4 changes: 2 additions & 2 deletions resolver/resolver.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3266,7 +3266,7 @@ class ResolveTypeMembersAndFieldsWalk {
}
trees.clear();

workers.multiplexJob("resolveTypeParamsWalk", [&gs, inputq, outputq]() -> void {
workers.multiplexJob("resolveTypeParamsWalk", [&gs = as_const(gs), inputq, outputq]() -> void {
Timer timeit(gs.tracer(), "resolveTypeParamsWalkWorker");
ResolveTypeMembersAndFieldsWalk walk;
ResolveTypeMembersAndFieldsWorkerResult output;
Expand Down Expand Up @@ -4133,7 +4133,7 @@ vector<ast::ParsedFile> resolveSigs(core::GlobalState &gs, vector<ast::ParsedFil

trees.clear();

workers.multiplexJob("resolveSignaturesWalk", [&gs, inputq, outputq]() -> void {
workers.multiplexJob("resolveSignaturesWalk", [&gs = as_const(gs), inputq, outputq]() -> void {
ResolveSignaturesWalk walk;
ResolveSignaturesWalk::ResolveSignaturesWalkResult output;
ast::ParsedFile job;
Expand Down
0