Fix `Key` interning race. #12152

jsirois · 2021-05-30T19:07:57Z

Previously a Key could be observed before it was added to the
reverse_keys mapping leading to a panic in key_get. That panic used
a debug format on the problematic key and the Debug impl for Key
indirectly uses key_get. This results in a panic while handling a
panic.

Fix the root cause of the panic, ensuring the panic can never happen
as was intended. Also fix the recursive use of key_get in the panic
for sanity sake.

Fixes #11926

[ci skip-build-wheels]

jsirois · 2021-05-30T19:09:31Z

The race sequencing and the coredump details are here: https://github.com/pantsbuild/pants/pull/11262/files#r642013320

Previously a `Key` could be observed before it was added to the `reverse_keys` mapping leading to a panic in key_get. That panic used a debug format on the problematic key and the `Debug` impl for `Key` indirectly uses `key_get`. This results in a panic while handling a panic. Fix the root cause of the panic, ensuring the panic can never happen as was intended. Also fix the recursive use of `key_get` in the panic for sanity sake. Fixes pantsbuild#11926 [ci skip-build-wheels]

jsirois · 2021-05-31T05:29:32Z

Although this fix is needed for correctness, I had never measured the perf impact. #11262 noted a ~5% speedup for ./pants dependencies --transitive :: with interning implemented.

This change looks like its affect on that benchmark is in the noise. Here ~/dev/pantsbuild/pants is master and ~/dev/pantsbuild/jsirois-pants is this PR:

With pantsd:

$ hyperfine -w 2 -L 'clone' 'pants,jsirois-pants' 'cd ~/dev/pantsbuild/{clone} && ./pants dependencies --transitive ::'
Benchmark #1: cd ~/dev/pantsbuild/pants && ./pants dependencies --transitive ::
  Time (mean ± σ):      3.012 s ±  0.055 s    [User: 1.048 s, System: 0.047 s]
  Range (min … max):    2.945 s …  3.100 s    10 runs
 
Benchmark #2: cd ~/dev/pantsbuild/jsirois-pants && ./pants dependencies --transitive ::
  Time (mean ± σ):      2.976 s ±  0.050 s    [User: 1.033 s, System: 0.053 s]
  Range (min … max):    2.907 s …  3.051 s    10 runs
 
Summary
  'cd ~/dev/pantsbuild/jsirois-pants && ./pants dependencies --transitive ::' ran
    1.01 ± 0.03 times faster than 'cd ~/dev/pantsbuild/pants && ./pants dependencies --transitive ::'

Without pantsd:

$ hyperfine -w 2 -L 'clone' 'pants,jsirois-pants' 'cd ~/dev/pantsbuild/{clone} && ./pants --no-pantsd dependencies --transitive ::'
Benchmark #1: cd ~/dev/pantsbuild/pants && ./pants --no-pantsd dependencies --transitive ::
  Time (mean ± σ):     10.650 s ±  0.085 s    [User: 12.206 s, System: 3.347 s]
  Range (min … max):   10.547 s … 10.832 s    10 runs
 
Benchmark #2: cd ~/dev/pantsbuild/jsirois-pants && ./pants --no-pantsd dependencies --transitive ::
  Time (mean ± σ):     10.605 s ±  0.181 s    [User: 12.170 s, System: 3.287 s]
  Range (min … max):   10.451 s … 10.944 s    10 runs
 
Summary
  'cd ~/dev/pantsbuild/jsirois-pants && ./pants --no-pantsd dependencies --transitive ::' ran
    1.00 ± 0.02 times faster than 'cd ~/dev/pantsbuild/pants && ./pants --no-pantsd dependencies --transitive ::'

Eric-Arellano

Thank you! 🙌

Could you please cherry-pick to 2.4 and 2.5? I will get out release candidates with this.

src/rust/engine/src/interning.rs

jsirois · 2021-06-01T18:31:14Z

Could you please cherry-pick to 2.4 and 2.5? I will get out release candidates with this.

Will do. You cut 2.3.2 and 2.2.3 on May 7th and this bug goes back to 2.2.x. That implies we're treating 2.2.x and greater as maintained still and so this cherrypick should got to those two as well. I'll optimistically prepare those and you can shoot down if there is something I'm missing.

Previously a `Key` could be observed before it was added to the `reverse_keys` mapping leading to a panic in `key_get`. That panic used a debug format on the problematic key and the `Debug` impl for `Key` indirectly uses `key_get`. This results in a panic while handling a panic. Fix the root cause of the panic, ensuring the panic can never happen as was intended. Also fix the recursive use of `key_get` in the panic for sanity sake. Fixes pantsbuild#11926 (cherry picked from commit a59be85) # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano · 2021-06-01T18:45:26Z

Sure, 2.3 and 2.2 sound great as well. Thanks

Previously a `Key` could be observed before it was added to the `reverse_keys` mapping leading to a panic in `key_get`. That panic used a debug format on the problematic key and the `Debug` impl for `Key` indirectly uses `key_get`. This results in a panic while handling a panic. Fix the root cause of the panic, ensuring the panic can never happen as was intended. Also fix the recursive use of `key_get` in the panic for sanity sake. Fixes pantsbuild#11926 (cherry picked from commit a59be85) [ci skip-build-wheels]

Previously a `Key` could be observed before it was added to the `reverse_keys` mapping leading to a panic in `key_get`. That panic used a debug format on the problematic key and the `Debug` impl for `Key` indirectly uses `key_get`. This results in a panic while handling a panic. Fix the root cause of the panic, ensuring the panic can never happen as was intended. Also fix the recursive use of `key_get` in the panic for sanity sake. Fixes pantsbuild#11926 (cherry picked from commit a59be85) # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Previously a `Key` could be observed before it was added to the `reverse_keys` mapping leading to a panic in `key_get`. That panic used a debug format on the problematic key and the `Debug` impl for `Key` indirectly uses `key_get`. This results in a panic while handling a panic. Fix the root cause of the panic, ensuring the panic can never happen as was intended. Also fix the recursive use of `key_get` in the panic for sanity sake. Fixes pantsbuild#11926 (cherry picked from commit a59be85) [ci skip-build-wheels]

Previously a `Key` could be observed before it was added to the `reverse_keys` mapping leading to a panic in `key_get`. That panic used a debug format on the problematic key and the `Debug` impl for `Key` indirectly uses `key_get`. This results in a panic while handling a panic. Fix the root cause of the panic, ensuring the panic can never happen as was intended. Also fix the recursive use of `key_get` in the panic for sanity sake. Fixes #11926 (cherry picked from commit a59be85)

Previously a `Key` could be observed before it was added to the `reverse_keys` mapping leading to a panic in `key_get`. That panic used a debug format on the problematic key and the `Debug` impl for `Key` indirectly uses `key_get`. This results in a panic while handling a panic. Fix the root cause of the panic, ensuring the panic can never happen as was intended. Also fix the recursive use of `key_get` in the panic for sanity sake. Fixes pantsbuild#11926 (cherry picked from commit a59be85) [ci skip-build-wheels]

Previously a `Key` could be observed before it was added to the `reverse_keys` mapping leading to a panic in `key_get`. That panic used a debug format on the problematic key and the `Debug` impl for `Key` indirectly uses `key_get`. This results in a panic while handling a panic. Fix the root cause of the panic, ensuring the panic can never happen as was intended. Also fix the recursive use of `key_get` in the panic for sanity sake. Fixes #11926 (cherry picked from commit a59be85)

This follows up on pantsbuild#12152. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Previously a `Key` could be observed before it was added to the `reverse_keys` mapping leading to a panic in `key_get`. That panic used a debug format on the problematic key and the `Debug` impl for `Key` indirectly uses `key_get`. This results in a panic while handling a panic. Fix the root cause of the panic, ensuring the panic can never happen as was intended. Also fix the recursive use of `key_get` in the panic for sanity sake. Fixes pantsbuild#11926 (cherry picked from commit a59be85) [ci skip-build-wheels]

This follows up on #12152.

Previously a `Key` could be observed before it was added to the `reverse_keys` mapping leading to a panic in `key_get`. That panic used a debug format on the problematic key and the `Debug` impl for `Key` indirectly uses `key_get`. This results in a panic while handling a panic. Fix the root cause of the panic, ensuring the panic can never happen as was intended. Also fix the recursive use of `key_get` in the panic for sanity sake. Fixes #11926 (cherry picked from commit a59be85)

jsirois · 2021-06-02T02:21:30Z

Now cherry-picked to 2.{2,3,4,5}.x.

jsirois requested review from Eric-Arellano and tdyas May 30, 2021 19:08

jsirois requested a review from benjyw May 30, 2021 19:09

jsirois force-pushed the issues/11926/one-cause branch from b42580d to 82ccb94 Compare May 30, 2021 20:23

tdyas approved these changes May 31, 2021

View reviewed changes

jsirois merged commit a59be85 into pantsbuild:main Jun 1, 2021

jsirois deleted the issues/11926/one-cause branch June 1, 2021 17:41

Eric-Arellano reviewed Jun 1, 2021

View reviewed changes

src/rust/engine/src/interning.rs Show resolved Hide resolved

jsirois added the needs-cherrypick label Jun 1, 2021

jsirois added a commit to jsirois/pants that referenced this pull request Jun 2, 2021

Explain Interns::key_get Key Debug sensitivity.

09ed6df

This follows up on pantsbuild#12152. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

jsirois mentioned this pull request Jun 2, 2021

Explain Interns::key_get Key Debug sensitivity. #12167

Merged

jsirois added a commit that referenced this pull request Jun 2, 2021

Explain Interns::key_get Key Debug sensitivity. (#12167)

0c1769e

This follows up on #12152.

jsirois removed the needs-cherrypick label Jun 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `Key` interning race. #12152

Fix `Key` interning race. #12152

jsirois commented May 30, 2021 •

edited

Loading

jsirois commented May 30, 2021

jsirois commented May 31, 2021

Eric-Arellano left a comment

jsirois commented Jun 1, 2021

Eric-Arellano commented Jun 1, 2021

jsirois commented Jun 2, 2021

Fix Key interning race. #12152

Fix Key interning race. #12152

Conversation

jsirois commented May 30, 2021 • edited Loading

jsirois commented May 30, 2021

jsirois commented May 31, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

jsirois commented Jun 1, 2021

Eric-Arellano commented Jun 1, 2021

jsirois commented Jun 2, 2021

Fix `Key` interning race. #12152

Fix `Key` interning race. #12152

jsirois commented May 30, 2021 •

edited

Loading