Skip to content
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.

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

Refactor ReferenceResolver to use native C++-enumerators in some places #4432

Merged
merged 14 commits into from
Feb 23, 2024

Conversation

asl
Copy link
Contributor

@asl asl commented Feb 16, 2024

Refactor ReferenceResolver to get rid of majority of unnecessary heap allocations due to obsessive Enumerator<> usage:

  • Use iterators and iterator ranges where possible
  • Ensure we do not allocate on heap temporary objects and exploit move semantics as much as possible returning vectors by value
  • Do some refactoring around Enumerator<>->toVector(): make it return result by value. It seems that the use in the tree is always about creating some temporaries. And it does not make any sense to allocate vectors on heap then
  • Few small improvements here and there, e.g. removing dependence from boost adapters here

Partially addresses #4412

Ideally, we'd switch from memory allocation at getDeclarations / getDeclsByName, but it's a bit long way to go for now.

@asl
Copy link
Contributor Author

asl commented Feb 16, 2024

Maybe it would make sense to wait until C++20 adoption and replace Enumerator with ranges and views over them.

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Generally I think this is a good change, thank you for your contribution. You may consider special-casing the common task of taking the first element of an enumerator in the enumerator itself. Do you have any performance measurements? I wonder what is the visible impact.

Comment on lines 85 to 86
auto decls = ns->getDeclsByName(name)->toVector();
if (!decls.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Idea: This pattern seems to be repeated often enough it might be good to add something like std::optional<T> first() or maybe T first() (with BUG_CHECK) + std::optional<T> optFirst() to Enumerator and use it instead of toVector() -- there is no point in creating a vector just to get first element (and even a singleton vector allocates on heap, while this would do no allocation).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I thought about this, but somehow decided to not to add... Will check it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, there is already single() method that does exactly this. It's just Enumerator interface is quite alien for C++ so noone seem to use is :)

auto *ctorCall =
new IR::ConstructorCallExpression(decl->srcInfo, decl->type, decl->arguments);
v.push_back(ctorCall);
const auto *decl = declVector[0]->to<IR::Declaration_Instance>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const auto *decl = declVector[0]->to<IR::Declaration_Instance>();
const auto *decl = declVector.front()->to<IR::Declaration_Instance>();

lib/iterator_range.h Outdated Show resolved Hide resolved
lib/iterator_range.h Show resolved Hide resolved
lib/iterator_range.h Outdated Show resolved Hide resolved
lib/iterator_range.h Outdated Show resolved Hide resolved
Comment on lines +31 to +32
// FIXME: should just insert()
for (const auto *decl : *nnDecls) decls.push_back(decl);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is not not possible to call insert now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the way how Enumerator is implemented.... Enumerator::begin() / Enumerator::end() return so-called EnumeratorHandle which does not satisfy iterator requirements (no trait types like reference / value_type, etc.)

frontends/common/resolveReferences/resolveReferences.cpp Outdated Show resolved Hide resolved
@vlstill
Copy link
Contributor

vlstill commented Feb 16, 2024

Maybe it would make sense to wait until C++20 adoption and replace Enumerator with ranges and views over them.

I was thinking about that too. I was also wondering if there is 3rd party ranges implementation that is (C++17) GCC 9 compatible. I think this change is worth it by itself, but I would not go over the entire codebase trying to root out the old enumerators now.

@asl
Copy link
Contributor Author

asl commented Feb 16, 2024

I wonder what is the visible impact.

We do not have much visible impact after memoization patch :) If we'd get rid of memoization, then there will be quite visible impact for large apps.

@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Feb 20, 2024
@asl asl requested review from vlstill and fruffy February 21, 2024 00:39
@asl asl force-pushed the decl-enum branch 3 times, most recently from e4a2a41 to 97ba34e Compare February 21, 2024 17:26
test/gtest/parser_unroll.cpp Outdated Show resolved Hide resolved
lib/iterator_range.h Show resolved Hide resolved
test/gtest/parser_unroll.cpp Show resolved Hide resolved
@fruffy
Copy link
Collaborator

fruffy commented Feb 21, 2024

@ChrisDodd Can you take a look at this PR?

@asl asl force-pushed the decl-enum branch 3 times, most recently from 7f5b8e8 to a7709c1 Compare February 23, 2024 05:30
@asl
Copy link
Contributor Author

asl commented Feb 23, 2024

I am going to merge this unless noone would object as this would allow us to get rid additional boost bits.

@asl asl force-pushed the decl-enum branch 2 times, most recently from c281353 to 8385379 Compare February 23, 2024 21:09
@asl asl enabled auto-merge (squash) February 23, 2024 21:57
@asl asl merged commit e14d108 into p4lang:main Feb 23, 2024
16 checks passed
@asl asl deleted the decl-enum branch February 23, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants