Skip to content
New issue

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

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

Already on GitHub? # to your account

module registry observer #450

Merged
merged 1 commit into from
Mar 24, 2023
Merged

module registry observer #450

merged 1 commit into from
Mar 24, 2023

Conversation

mikea
Copy link
Contributor

@mikea mikea commented Mar 15, 2023

enables recording timing/memory/usage information

EW PR 5129

@mikea mikea force-pushed the mikea/module-registry-observer branch from f8e2eb0 to c8ed33a Compare March 15, 2023 21:46
@mikea mikea force-pushed the mikea/module-registry-observer branch from c8ed33a to 32f236b Compare March 23, 2023 18:11
@mikea mikea changed the title wip: module registry observer module registry observer Mar 23, 2023
@mikea mikea marked this pull request as ready for review March 23, 2023 18:26
@mikea mikea force-pushed the mikea/module-registry-observer branch from 32f236b to 4e5f934 Compare March 23, 2023 18:35
ModuleInfoCompileOption option,
const ModuleRegistryObserver& observer) {
// destroy the observer after compilation finished to indicate the end of the process.
auto compilationObserver = observer.onEsmCompilationStart(js.v8Isolate, name, option);
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand what could be happening here: an observer returns an owned which is an opaque container (kj::Own<void>) for a class / struct that "observes" the start and end of compilation events when the opaque container is created and destroyed. A ModuleRegisryObserver implementation can choose what happens on start and end of compilation. The LHS is doing the observing the end of the compilation on behalf of the ModuleRegistryObserver, e.g. compilationEndObserver.

I might call compilationObserver something like compilationEvent / compilationLifetime

Copy link
Contributor Author

@mikea mikea Mar 24, 2023

Choose a reason for hiding this comment

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

Thanks. I thought about it a little bit, and decided to keep the name observer.

Couple reasons:

  • we already use destructor in observers to send an event, so this usage/naming is consistent
  • this observer is very simple now (only destructor), but may get additional methods in the future or got rid of the destructor altogether if we need to send more information (e.g. successful/non-successful)

Let me know if you feel strongly about this one.

@mikea mikea merged commit c46a30d into main Mar 24, 2023
@mikea mikea deleted the mikea/module-registry-observer branch March 24, 2023 15:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants