Skip to content

Conversation

@rintaro
Copy link
Member

@rintaro rintaro commented Jun 16, 2023

Iterating all options and potential file system access is not great for every plugin lookup request. Instead, lazily create a single lookup table keyed by module name.

@rintaro
Copy link
Member Author

rintaro commented Jun 16, 2023

@swift-ci Please smoke test

@rintaro rintaro requested a review from bnbarham June 16, 2023 22:07
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
auto found = getPluginMap().find(moduleName);
auto found = map.find(moduleName);

@rintaro rintaro force-pushed the macros-pluginloader-table branch from 5ba7f10 to 16386aa Compare June 16, 2023 22:41
@rintaro
Copy link
Member Author

rintaro commented Jun 16, 2023

@swift-ci Please smoke test

Copy link
Contributor

Choose a reason for hiding this comment

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

Thought's on making this its own type?

struct PluginEntry {
  StringRef libPath;
  StringRef executablePath;
};

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
/// Get plug module name from \p path if the path looks like a shared library
/// Get plugin module name from \p path if the path looks like a shared library

Comment on lines 52 to 53
Copy link
Contributor

Choose a reason for hiding this comment

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

Can... we just not 😅?

Copy link
Contributor

Choose a reason for hiding this comment

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

To expand on this - sure, we might have an identifier in the map that isn't a valid. But we wouldn't be able to look it up, so it seems harmless to me.

@rintaro rintaro force-pushed the macros-pluginloader-table branch from 16386aa to 4ca6bac Compare June 16, 2023 23:05
@rintaro
Copy link
Member Author

rintaro commented Jun 16, 2023

@swift-ci Please smoke test

@rintaro rintaro force-pushed the macros-pluginloader-table branch from 4ca6bac to a27646f Compare June 16, 2023 23:22
@rintaro
Copy link
Member Author

rintaro commented Jun 16, 2023

@swift-ci Please smoke test

Iterating all options and potential file system access is not great for
every plugin lookup request. Instead, lazily create a single lookup table
keyed by module name.
@rintaro rintaro force-pushed the macros-pluginloader-table branch from a27646f to b8a073c Compare June 17, 2023 01:17
@rintaro
Copy link
Member Author

rintaro commented Jun 17, 2023

@swift-ci Please smoke test

@xedin xedin removed their request for review June 20, 2023 16:50
@rintaro rintaro merged commit a22b71f into swiftlang:main Jun 20, 2023
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