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

review: perf: reduce overhead of isLocalType check #4211

Merged
merged 2 commits into from
Oct 8, 2021

Conversation

SirYwell
Copy link
Collaborator

@SirYwell SirYwell commented Oct 7, 2021

Those are two very related performance improvements. While they aren't that significant, it might add up for larger projects.

  1. When using TypeFactory#get(String) with a nested type, the method extracts the outer type string, gets that type recursively and uses its reference to check if it is a local type. The implementation of isLocalType then accesses its type again and calls the isLocalType method of the type. We can just skip this indirection by calling isLocalType on the type directly.

  2. Previously, the isLocalType implementation of the reference accessed the declaration twice (= building the qualified name and passing it to TypeFactory#get(String) as mentioned in 1.) This is only needed once, so we reuse the returned value.

I also saw that if the declaration in 2. is null, there is a regex pattern

final Pattern pattern = Pattern.compile("^([0-9]+)([a-zA-Z]+)$");
final Matcher m = pattern.matcher(getSimpleName());
return m.find();
which isn't efficient in itself. I didn't change that because the implementation is also wrong here, because something like 1LocalType1 would not be matched. I will address that separately.

@SirYwell SirYwell changed the title wip: perf: reduce overhead of isLocalType check review: perf: reduce overhead of isLocalType check Oct 7, 2021
Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @SirYwell. Hard to gauge how much of a performance improvement this provides, but it's definitely not going to be worse and reads a bit better to boot.

@slarse slarse merged commit c257ed0 into INRIA:master Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants