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

Update rustc-hash to version 2 #17954

Merged
merged 4 commits into from
Oct 21, 2024
Merged

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Aug 24, 2024

This brings in the new optimized algorithm that was shown to have small performance benefits for rustc. I haven't run the rust-analyzer benchmarks.

See rust-lang/rustc-hash#37.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2024
@Noratrieb
Copy link
Member Author

oh lord, it looks like something relies on the iteration order or something like that :3.

@Veykril
Copy link
Member

Veykril commented Aug 25, 2024

Yup, the iteration here calls a mutating callback and the state there seems to be iteration dependent (sorting the iterator by key causes the same issue on master)

cm.memory.iter().map(|(addr, val)| transform((addr, val))).collect()

@Veykril
Copy link
Member

Veykril commented Aug 25, 2024

The funny thing is, that test apparently should succeed given rustc is fine with it?

        pub enum TagTree {
            Leaf,
            Choice(&'static [TagTree]),
        }
        const GOAL: TagTree = {
            const TAG_TREE: TagTree = TagTree::Choice(&[
                {
                    const VARIANT_TAG_TREE: TagTree = TagTree::Choice(
                        &[
                            TagTree::Leaf,
                        ],
                    );
                    VARIANT_TAG_TREE
                },
            ]);
            TAG_TREE
        };
        ```
        compiles just fine, yet the test errors on master

@Veykril
Copy link
Member

Veykril commented Aug 25, 2024

Naively I'd say, lets replace the FxHashMap at

vtable: VTableMap,
with an FxIndexMap and change the expected test output to success (if that change makes it succeed)

@HKalbasi
Copy link
Member

The test looks strange. I guess it meant to do recursive constants, something like this:

pub enum TagTree {
    Leaf,
    Choice(&'static [TagTree]),
}
const GOAL: TagTree = {
    const TAG_TREE: TagTree = TagTree::Choice(&[
        {
            const VARIANT_TAG_TREE: TagTree = TagTree::Choice(
                &[
                    TAG_TREE,
                ],
            );
            VARIANT_TAG_TREE
        },
    ]);
    TAG_TREE
};

The failure of the original test clearly is a bug. It should work with any order of execution, and sorting or using an stable iteration order does not fix the underlying problem. I would suggest changing the test to use an actual recursive constant like the code above, and opening an issue for that bug for further investigation.

@Veykril Veykril force-pushed the rustc-blazing-hash branch 4 times, most recently from 9059aff to 6a78851 Compare September 4, 2024 08:50
@Veykril
Copy link
Member

Veykril commented Sep 4, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Sep 4, 2024

📌 Commit 6a78851 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 4, 2024

⌛ Testing commit 6a78851 with merge aee8b45...

bors added a commit that referenced this pull request Sep 4, 2024
Update rustc-hash to version 2

This brings in the new optimized algorithm that was shown to have small performance benefits for rustc. I haven't run the rust-analyzer benchmarks.

See rust-lang/rustc-hash#37.
@bors
Copy link
Contributor

bors commented Sep 4, 2024

💔 Test failed - checks-actions

@bors
Copy link
Contributor

bors commented Sep 20, 2024

☔ The latest upstream changes (presumably #18151) made this pull request unmergeable. Please resolve the merge conflicts.

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2024
Noratrieb and others added 3 commits October 21, 2024 11:28
This brings in the new optimized algorithm that was shown to have small performance benefits for
rustc.
@Veykril Veykril force-pushed the rustc-blazing-hash branch 2 times, most recently from 9a94802 to 8be3b80 Compare October 21, 2024 09:42
@Veykril
Copy link
Member

Veykril commented Oct 21, 2024

Okay let's see what breaks
@bors r+

@bors
Copy link
Contributor

bors commented Oct 21, 2024

📌 Commit 8be3b80 has been approved by Veykril

It is now in the queue for this repository.

bors added a commit that referenced this pull request Oct 21, 2024
Update rustc-hash to version 2

This brings in the new optimized algorithm that was shown to have small performance benefits for rustc. I haven't run the rust-analyzer benchmarks.

See rust-lang/rustc-hash#37.
@bors
Copy link
Contributor

bors commented Oct 21, 2024

⌛ Testing commit 8be3b80 with merge 102a3b1...

@bors
Copy link
Contributor

bors commented Oct 21, 2024

💔 Test failed - checks-actions

@Veykril
Copy link
Member

Veykril commented Oct 21, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Oct 21, 2024

📌 Commit 02e189d has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 21, 2024

⌛ Testing commit 02e189d with merge fb832ff...

@bors
Copy link
Contributor

bors commented Oct 21, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing fb832ff to master...

@bors bors merged commit fb832ff into rust-lang:master Oct 21, 2024
11 checks passed
@Noratrieb Noratrieb deleted the rustc-blazing-hash branch October 21, 2024 12:26
@Noratrieb
Copy link
Member Author

nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants