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

Fix fn dedup miscompilation leading to MemoryOverflow on Vecs #6195

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Jun 27, 2024

Description

This PR fixes a miscompilation bug caused by our dedup cache just before IR generation, we started to notice this bug with some MemoryOverflow reverts on Vec tests using 0.61.

These reverts happen because Vec::with_capacity allocates 16 bytes per item, but later one Vec::push pushes 24 bytes per item; causing some writes outside the allocated buffer.

This is happening because, before we start IR generation, we try to coalesce identical functions using the following hash key: https://github.com/FuelLabs/sway/blob/master/sway-core/src/ir_generation.rs#L75

The example below shows that two different bodies end up colliding at the same key.

Implementing for Some(Vec<[u64; 2]>)
Not parts of the key
    Key: 16648822319765008093
    Body hash: 14066144736883705909
Parts of the key
    Span: Span { src (ptr): 0x5f096722d090, source_id: Some(SourceId { id: 2097176 }), start: 4886, end: 5032, as_str(): "pub fn with_capacity(capacity: u64) -> Self {\n        Self {\n            buf: RawVec::with_capacity(capacity),\n            len: 0,\n        }\n    }" }
    Parameters:
        u64
    Type parameters:

Implementing for Some(Vec<SomeEnum<u32>>)
Not parts of the key
    Key: 16648822319765008093
    Body hash: 17149806863110053630
Parts of the key
    Span: Span { src (ptr): 0x5f096722d090, source_id: Some(SourceId { id: 2097176 }), start: 4886, end: 5032, as_str(): "pub fn with_capacity(capacity: u64) -> Self {\n        Self {\n            buf: RawVec::with_capacity(capacity),\n            len: 0,\n        }\n    }" }
    Parameters:
        u64
    Type parameters:

The difference here is that in one case RawVec is inferred to be RawVec<Vec<[u64; 2]>> and in the other RawVec<Vec<SomeEnum<u32>>. The crux of the issue is that this difference does not appear in the key.

The question is why this broke with_capacity and not push. And the answer in that push has the self and the generic parameters to differentiate them, whilst with_capacity does not.

Implementing for Some(Vec<SomeEnum<u32>>)
Not parts of the key
    Key: 8342657797980723970
    Body hash: 328023847492220083
Parts of the key
    Span: Span { src (ptr): 0x5f096722d090, source_id: Some(SourceId { id: 2097176 }), start: 5490, end: 5964, as_str(): "pub fn push(ref mut self, value: T) {\n        // If there is insufficient capacity, grow the buffer.\n        if self.len == self.buf.capacity() {\n            self.buf.grow();\n        };\n\n        // Get a pointer to the end of the buffer, where the new element will\n        // be inserted.\n        let end = self.buf.ptr().add::<T>(self.len);\n\n        // Write `value` at pointer `end`\n        end.write::<T>(value);\n\n        // Increment length.\n        self.len += 1;\n    }" }
    Parameters:
        Vec<SomeEnum<u32>>
        SomeEnum<u32>
    Type parameters:

Implementing for Some(Vec<SomeStruct<u32>>)
Not parts of the key
    Key: 15738668646378064265
    Body hash: 15996600191005707730
Parts of the key
    Span: Span { src (ptr): 0x5f096722d090, source_id: Some(SourceId { id: 2097176 }), start: 5490, end: 5964, as_str(): "pub fn push(ref mut self, value: T) {\n        // If there is insufficient capacity, grow the buffer.\n        if self.len == self.buf.capacity() {\n            self.buf.grow();\n        };\n\n        // Get a pointer to the end of the buffer, where the new element will\n        // be inserted.\n        let end = self.buf.ptr().add::<T>(self.len);\n\n        // Write `value` at pointer `end`\n        end.write::<T>(value);\n\n        // Increment length.\n        self.len += 1;\n    }" }
    Parameters:
        Vec<SomeStruct<u32>>
        SomeStruct<u32>
    Type parameters:

Funnily enough, we also have the inverse problem. Sometimes functions with the same body do not fall at the same key, because their TypeId are different, and HashWithEngines actually look at TypeInfo variants. This is probably increasing code size and/or throwing more stuff at the optimizer.

Implementing for Some(Vec<SomeEnum<u32>>)
Not parts of the key
    Key: 12091758953823056676
    Body hash: 328023847492220083
Parts of the key
    Span: Span { src (ptr): 0x5f096722d090, source_id: Some(SourceId { id: 2097176 }), start: 5490, end: 5964, as_str(): "pub fn push(ref mut self, value: T) {\n        // If there is insufficient capacity, grow the buffer.\n        if self.len == self.buf.capacity() {\n            self.buf.grow();\n        };\n\n        // Get a pointer to the end of the buffer, where the new element will\n        // be inserted.\n        let end = self.buf.ptr().add::<T>(self.len);\n\n        // Write `value` at pointer `end`\n        end.write::<T>(value);\n\n        // Increment length.\n        self.len += 1;\n    }" }
    Parameters:
        Vec<SomeEnum<u32>>
        SomeEnum<u32>
    Type parameters:


Implementing for Some(Vec<SomeEnum<u32>>)
Not parts of the key
    Key: 8342657797980723970
    Body hash: 328023847492220083
Parts of the key
    Span: Span { src (ptr): 0x5f096722d090, source_id: Some(SourceId { id: 2097176 }), start: 5490, end: 5964, as_str(): "pub fn push(ref mut self, value: T) {\n        // If there is insufficient capacity, grow the buffer.\n        if self.len == self.buf.capacity() {\n            self.buf.grow();\n        };\n\n        // Get a pointer to the end of the buffer, where the new element will\n        // be inserted.\n        let end = self.buf.ptr().add::<T>(self.len);\n\n        // Write `value` at pointer `end`\n        end.write::<T>(value);\n\n        // Increment length.\n        self.len += 1;\n    }" }
    Parameters:
        Vec<SomeEnum<u32>>
        SomeEnum<u32>
    Type parameters:

My solution for this problem is to use the body hash as the cache key. We can try to be less risky and include the body hash and keep the other items.

Below there are two graphs showing that compilation time and binary size are not affected too much by this change.

Compilation Time

image

Binary Size

image

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@xunilrj xunilrj force-pushed the xunilrj/fix-vec-memoverflow branch from 2660a01 to fa75266 Compare June 27, 2024 12:20
Copy link

Benchmark for 8f8f743

Click to view benchmark
Test Base PR %
code_action 5.3±0.09ms 5.3±0.26ms 0.00%
code_lens 282.8±9.55ns 284.7±4.80ns +0.67%
compile 2.6±0.05s 2.6±0.03s 0.00%
completion 5.0±0.19ms 4.9±0.23ms -2.00%
did_change_with_caching 2.5±0.03s 2.6±0.03s +4.00%
document_symbol 931.8±36.27µs 860.3±14.12µs -7.67%
format 83.1±0.82ms 82.7±1.50ms -0.48%
goto_definition 343.8±16.35µs 344.0±10.76µs +0.06%
highlight 9.2±0.16ms 9.0±0.07ms -2.17%
hover 490.9±4.80µs 490.4±3.52µs -0.10%
idents_at_position 121.5±0.40µs 122.0±1.18µs +0.41%
inlay_hints 642.8±19.04µs 649.4±42.25µs +1.03%
on_enter 484.1±19.26ns 474.9±17.63ns -1.90%
parent_decl_at_position 3.8±0.06ms 3.7±0.05ms -2.63%
prepare_rename 343.1±5.33µs 341.2±13.94µs -0.55%
rename 9.5±0.18ms 9.4±0.22ms -1.05%
semantic_tokens 1190.1±12.70µs 1281.2±21.13µs +7.65%
token_at_position 334.2±2.37µs 346.4±3.20µs +3.65%
tokens_at_position 3.8±0.07ms 3.7±0.02ms -2.63%
tokens_for_file 394.8±2.09µs 461.5±11.62µs +16.89%
traverse 38.5±0.91ms 38.5±0.30ms 0.00%

Copy link

Benchmark for 53fa851

Click to view benchmark
Test Base PR %
code_action 5.2±0.30ms 5.2±0.01ms 0.00%
code_lens 283.4±7.30ns 287.3±9.66ns +1.38%
compile 2.5±0.03s 2.5±0.04s 0.00%
completion 4.7±0.01ms 4.6±0.05ms -2.13%
did_change_with_caching 2.5±0.02s 2.4±0.04s -4.00%
document_symbol 867.7±13.96µs 866.5±16.69µs -0.14%
format 82.5±1.04ms 82.8±0.99ms +0.36%
goto_definition 338.0±6.10µs 338.4±8.09µs +0.12%
highlight 9.1±0.11ms 9.0±0.10ms -1.10%
hover 489.8±5.27µs 491.4±6.00µs +0.33%
idents_at_position 121.4±0.43µs 119.9±1.12µs -1.24%
inlay_hints 641.3±7.33µs 656.6±38.42µs +2.39%
on_enter 463.1±7.39ns 511.9±9.00ns +10.54%
parent_decl_at_position 3.7±0.05ms 3.7±0.02ms 0.00%
prepare_rename 333.7±5.50µs 338.3±6.18µs +1.38%
rename 9.3±0.08ms 9.2±0.05ms -1.08%
semantic_tokens 1301.4±10.31µs 1257.1±12.70µs -3.40%
token_at_position 333.7±3.09µs 335.5±3.88µs +0.54%
tokens_at_position 3.7±0.03ms 3.7±0.04ms 0.00%
tokens_for_file 404.8±18.27µs 404.3±2.25µs -0.12%
traverse 37.0±0.55ms 37.5±0.75ms +1.35%

@xunilrj xunilrj marked this pull request as ready for review June 27, 2024 15:24
@xunilrj xunilrj requested a review from a team as a code owner June 27, 2024 15:24
@xunilrj xunilrj changed the title Xunilrj/fix vec memoverflow Fix fn dedup miscompilation leading to MemoryOverflow on Vecs Jun 27, 2024
Copy link
Contributor

@IGI-111 IGI-111 left a comment

Choose a reason for hiding this comment

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

Nice catch!

Copy link

Benchmark for 559e04b

Click to view benchmark
Test Base PR %
code_action 5.2±0.08ms 5.0±0.09ms -3.85%
code_lens 279.7±8.24ns 280.9±12.54ns +0.43%
compile 2.5±0.02s 2.5±0.04s 0.00%
completion 4.7±0.09ms 4.5±0.07ms -4.26%
did_change_with_caching 2.4±0.02s 2.4±0.03s 0.00%
document_symbol 876.9±22.13µs 972.1±38.55µs +10.86%
format 73.9±1.16ms 73.6±1.34ms -0.41%
goto_definition 365.9±7.15µs 340.6±7.38µs -6.91%
highlight 9.0±0.02ms 8.7±0.18ms -3.33%
hover 513.7±7.57µs 494.3±14.36µs -3.78%
idents_at_position 118.4±0.34µs 120.1±0.50µs +1.44%
inlay_hints 642.2±10.76µs 635.5±29.20µs -1.04%
on_enter 479.6±14.67ns 466.3±15.31ns -2.77%
parent_decl_at_position 3.7±0.04ms 3.5±0.04ms -5.41%
prepare_rename 358.9±6.06µs 341.4±4.54µs -4.88%
rename 9.2±0.11ms 8.9±0.19ms -3.26%
semantic_tokens 1286.3±15.38µs 1290.1±9.00µs +0.30%
token_at_position 336.9±1.94µs 330.5±2.57µs -1.90%
tokens_at_position 3.7±0.05ms 3.5±0.05ms -5.41%
tokens_for_file 403.4±3.19µs 398.7±2.40µs -1.17%
traverse 38.0±0.69ms 37.9±1.25ms -0.26%

@xunilrj xunilrj merged commit 03c09bf into master Jun 27, 2024
38 checks passed
@xunilrj xunilrj deleted the xunilrj/fix-vec-memoverflow branch June 27, 2024 16:22
@hal3e hal3e restored the xunilrj/fix-vec-memoverflow branch June 28, 2024 07:43
@danielbate danielbate self-assigned this Jun 28, 2024
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.

4 participants