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

allow some cool down time after allocation measurements [SHOULD FIX TEST FAILURES] #257

Merged
merged 1 commit into from
Mar 30, 2018

Conversation

weissi
Copy link
Member

@weissi weissi commented Mar 29, 2018

Motivation:

We have code running on multiple threads and certain allocations/frees
happen for example after a promise has been fulfilled. We can't
deterministically wait for that so the only thing we can do is sleep a
little and wait...

Modifications:

Added a 100ms delay after an allocation counted test run.

Result:

Hopefully we don't see this again:

["total_allocations": 322051, "remaining_allocations": 24], ["total_allocations": 322051, "remaining_allocations": -24]

where the first test run had 24 allocations that weren't freed and the
second one on the other hand had 24 frees for allocations that "didn't
happen"...

Motivation:

We have code running on multiple threads and certain allocations/frees
happen for example after a promise has been fulfilled. We can't
deterministically wait for that so the only thing we can do is sleep a
little and wait...

Modifications:

Added a 100ms delay after an allocation counted test run.

Result:

Hopefully we don't see this again:

    ["total_allocations": 322051, "remaining_allocations": 24], ["total_allocations": 322051, "remaining_allocations": -24]

where the first test run had 24 allocations that weren't freed and the
second one on the other hand had 24 frees for allocations that "didn't
happen"...
@weissi
Copy link
Member Author

weissi commented Mar 30, 2018

@tomerd this seems to be quite important, just tested it locally and even after 50 runs I always got exactly 0 remaining allocations 🙌. I’d merge this asap as it will (hopefully) stop other PRs going red. Sorry about that...

@weissi weissi changed the title allow some cool down time after allocation measurements allow some cool down time after allocation measurements [SHOULD FIX TEST FAILURES] Mar 30, 2018
Copy link
Member

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

seams reasonable, unless there is a more deterministic way to know how long to wait

@normanmaurer normanmaurer merged commit 17c5114 into apple:master Mar 30, 2018
@normanmaurer
Copy link
Member

Merged thanks

@weissi weissi deleted the jw-cool-down branch March 30, 2018 14:48
@Lukasa Lukasa added the semver/patch No public API change. label Apr 9, 2018
@Lukasa Lukasa added this to the 1.4.0 milestone Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants