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

Noop for test GC pass when not in debug #792

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Conversation

Warfields
Copy link
Contributor

To get accurate metrics when load testing an optimized build, the GC pass that is normally done per test is now disabled.

@Warfields Warfields requested review from jclee and kentonv June 20, 2023 22:16
@@ -194,12 +194,14 @@ void Lock::setLoggerCallback(kj::Function<Logger>&& logger) {
}

void Lock::requestGcForTesting() const {
#ifdef KJ_DEBUG
Copy link
Member

Choose a reason for hiding this comment

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

It'd be better to do this in WorkerEntrypoint::maybeAddGcPassForTest(). Otherwise, there's still a bunch of extra work that happens there in predictable mode, in particular taking an isolate lock, which will slow down load tests for no reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

To get accurate metrics when load testing an optimized build, the GC pass
that is normally done per test is now disabled.
@Warfields Warfields force-pushed the swarfield/debug-mode-for-gc branch from c0cdfe9 to 952080d Compare June 20, 2023 22:20
Copy link
Contributor

@jclee jclee left a comment

Choose a reason for hiding this comment

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

Seems OK for the immediate need. At some point in the future, might be nice to plumb in a separate GC-after-test option, so that debug and release tests have more similar behavior. Also, not sure if asan tests are built in release mode or not, but being able to run those with or without GC might increase the chances of finding lifetime errors.

@Warfields
Copy link
Contributor Author

Seems OK for the immediate need. At some point in the future, might be nice to plumb in a separate GC-after-test option, so that debug and release tests have more similar behavior. Also, not sure if asan tests are built in release mode or not, but being able to run those with or without GC might increase the chances of finding lifetime errors.

ASAN is not currently built in release.

@Warfields Warfields merged commit 96876a6 into main Jun 20, 2023
@Warfields Warfields deleted the swarfield/debug-mode-for-gc branch June 20, 2023 22:50
# 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.

4 participants