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

Drop race detector from Travis builds #474

Merged
merged 1 commit into from
Aug 19, 2018
Merged

Drop race detector from Travis builds #474

merged 1 commit into from
Aug 19, 2018

Conversation

rtfb
Copy link
Collaborator

@rtfb rtfb commented Aug 18, 2018

We do not have a single go statement in Blackfriday code, -race does
nothing for us, but burn CPU cycles (and it hurts because we're hitting
timeouts on full test runs).

We do not have a single go statement in Blackfriday code, -race does
nothing for us, but burn CPU cycles (and it hurts because we're hitting
timeouts on full test runs).
@rtfb rtfb requested a review from dmitshur August 18, 2018 20:20
@rtfb
Copy link
Collaborator Author

rtfb commented Aug 18, 2018

@dmitshur, please take a look. I propose we drop -race from Travis runs. With #428 I'm doubling the amount of tests and we're hitting the default 10m timeout very hard. As far as I can tell, -race does nothing for us, since we have zero parallelism, it only makes tests more than 10x slower (~420s vs ~40s on my laptop).

Copy link
Collaborator

@dmitshur dmitshur left a comment

Choose a reason for hiding this comment

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

The arguments to drop it, in this specific case at this time, SGTM. However, I have two followup points:

  1. I think all reviewers merging future PRs will need to be mindful of this, and if there's ever a go keyword added to the codebase, we should actively consider re-adding -race.

  2. I don't see a reason why we shouldn't be running tests in parallel by using t.Parallel(). We're currently not doing that at all. From a quick local test on a quad-core laptop, allowing tests to run in parallel speeds up go test -race gopkg.in/russross/blackfriday.v2 from 156.058s to 77.826s. I'm seeing only 200% CPU usage for most of the time (out of 800% max, given 8 threads), so I suspect there are 2 tests that take the longest to run, and can likely be sped up even further by breaking up into sub-tests that can run in parallel.

@rtfb rtfb mentioned this pull request Aug 19, 2018
@rtfb
Copy link
Collaborator Author

rtfb commented Aug 19, 2018

By zero parallelism I meant that all the logic of Blackfriday runs in a single goroutine, and there's no data that could race. But a point about speeding up tests by running them in parallel is very good one, I filed an issue for it.

Thanks!

@rtfb rtfb merged commit 5a12bbc into v2 Aug 19, 2018
@rtfb rtfb deleted the drop-race-detector branch August 19, 2018 11:08
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