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

Avoid symbol interning in `file_metadata`. #60973

Merged
merged 1 commit into from May 22, 2019

Conversation

Projects
None yet
7 participants
@nnethercote
Copy link
Contributor

commented May 20, 2019

This commit changes created_files so it uses strings directly as keys,
rather than symbols derived from the strings. This avoids the cost of
having to do the hash table lookups to produce the symbols from the
strings.

The commit also uses entry to avoid doing a repeated hash table lookup
(get + insert).

Note that PR #60467 improved this code somewhat; this is a further
improvement.

r? @davidtwco

Avoid symbol interning in `file_metadata`.
This commit changes `created_files` so it uses strings directly as keys,
rather than symbols derived from the strings. This avoids the cost of
having to do the hash table lookups to produce the symbols from the
strings.

The commit also uses `entry` to avoid doing a repeated hash table lookup
(`get` + `insert`).

Note that PR #60467 improved this code somewhat; this is a further
improvement.
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

⌛️ Trying commit c5d9401 with merge e11f859...

bors added a commit that referenced this pull request May 20, 2019

Auto merge of #60973 - nnethercote:fix-file_metadata-more, r=<try>
Avoid symbol interning in `file_metadata`.

This commit changes `created_files` so it uses strings directly as keys,
rather than symbols derived from the strings. This avoids the cost of
having to do the hash table lookups to produce the symbols from the
strings.

The commit also uses `entry` to avoid doing a repeated hash table lookup
(`get` + `insert`).

Note that PR #60467 improved this code somewhat; this is a further
improvement.

r? @davidtwco
@davidtwco

This comment has been minimized.

Copy link
Member

commented May 20, 2019

LGTM, but I'm not able to approve PRs.

r? @michaelwoerister

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@davidtwco: Has your status changed recently? You approved #60467, which was related.

@davidtwco

This comment has been minimized.

Copy link
Member

commented May 20, 2019

@davidtwco: Has your status changed recently?

It has. Can chat on Zulip if you want to know more.

@bors

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

☀️ Try build successful - checks-travis
Build commit: e11f859

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

@rust-timer

This comment has been minimized.

Copy link

commented May 20, 2019

Success: Queued e11f859 with parent caef1e8, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented May 20, 2019

Finished benchmarking try commit e11f859: comparison url

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

The perf results are weak. The new code is arguably cleaner, reducing the number of arguments. @michaelwoerister, I'll let you decide if it's worth taking.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

Yes, the new code is cleaner. Thanks, @nnethercote!

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

📌 Commit c5d9401 has been approved by michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

⌛️ Testing commit c5d9401 with merge b02dc3c...

bors added a commit that referenced this pull request May 21, 2019

Auto merge of #60973 - nnethercote:fix-file_metadata-more, r=michaelw…
…oerister

Avoid symbol interning in `file_metadata`.

This commit changes `created_files` so it uses strings directly as keys,
rather than symbols derived from the strings. This avoids the cost of
having to do the hash table lookups to produce the symbols from the
strings.

The commit also uses `entry` to avoid doing a repeated hash table lookup
(`get` + `insert`).

Note that PR #60467 improved this code somewhat; this is a further
improvement.

r? @davidtwco
@pietroalbini

This comment has been minimized.

Copy link
Member

commented May 21, 2019

@bors retry

Yielding priority to the beta release.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Cloning into 'rust-lang/rust'...
travis_time:end:275de31f:start=1558439731837882177,finish=1558439737701575122,duration=5863692945
$ cd rust-lang/rust
$ git checkout -qf b02dc3ce95855e249764e18415a05555ae4202c5
fatal: reference is not a tree: b02dc3ce95855e249764e18415a05555ae4202c5
The command "git checkout -qf b02dc3ce95855e249764e18415a05555ae4202c5" failed and exited with 128 during .
Your build has been stopped.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@bors retry

Centril added a commit to Centril/rust that referenced this pull request May 21, 2019

Rollup merge of rust-lang#60973 - nnethercote:fix-file_metadata-more,…
… r=michaelwoerister

Avoid symbol interning in `file_metadata`.

This commit changes `created_files` so it uses strings directly as keys,
rather than symbols derived from the strings. This avoids the cost of
having to do the hash table lookups to produce the symbols from the
strings.

The commit also uses `entry` to avoid doing a repeated hash table lookup
(`get` + `insert`).

Note that PR rust-lang#60467 improved this code somewhat; this is a further
improvement.

r? @davidtwco

bors added a commit that referenced this pull request May 21, 2019

Auto merge of #61009 - Centril:rollup-7l0lf9f, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #60581 (convert custom try macro to `?`)
 - #60963 (Update boxed::Box docs on memory layout)
 - #60973 (Avoid symbol interning in `file_metadata`.)
 - #60991 (LocalDecl push returns Local len)
 - #60998 (static_assert: make use of anonymous constants)
 - #61006 (adjust deprecation date of mem::uninitialized)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request May 21, 2019

Auto merge of #61009 - Centril:rollup-7l0lf9f, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #60581 (convert custom try macro to `?`)
 - #60963 (Update boxed::Box docs on memory layout)
 - #60973 (Avoid symbol interning in `file_metadata`.)
 - #60991 (LocalDecl push returns Local len)
 - #60998 (static_assert: make use of anonymous constants)
 - #61006 (adjust deprecation date of mem::uninitialized)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request May 21, 2019

Rollup merge of rust-lang#60973 - nnethercote:fix-file_metadata-more,…
… r=michaelwoerister

Avoid symbol interning in `file_metadata`.

This commit changes `created_files` so it uses strings directly as keys,
rather than symbols derived from the strings. This avoids the cost of
having to do the hash table lookups to produce the symbols from the
strings.

The commit also uses `entry` to avoid doing a repeated hash table lookup
(`get` + `insert`).

Note that PR rust-lang#60467 improved this code somewhat; this is a further
improvement.

r? @davidtwco

bors added a commit that referenced this pull request May 21, 2019

Auto merge of #61016 - Centril:rollup-5qj20z8, r=Centril
Rollup of 9 pull requests

Successful merges:

 - #60581 (convert custom try macro to `?`)
 - #60963 (Update boxed::Box docs on memory layout)
 - #60973 (Avoid symbol interning in `file_metadata`.)
 - #60982 (Do not fail on child without DefId)
 - #60991 (LocalDecl push returns Local len)
 - #60995 (Add stream_to_parser_with_base_dir)
 - #60998 (static_assert: make use of anonymous constants)
 - #61005 (Emit error when trying to use PGO in conjunction with unwinding on Windows.)
 - #61006 (adjust deprecation date of mem::uninitialized)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request May 21, 2019

Rollup merge of rust-lang#60973 - nnethercote:fix-file_metadata-more,…
… r=michaelwoerister

Avoid symbol interning in `file_metadata`.

This commit changes `created_files` so it uses strings directly as keys,
rather than symbols derived from the strings. This avoids the cost of
having to do the hash table lookups to produce the symbols from the
strings.

The commit also uses `entry` to avoid doing a repeated hash table lookup
(`get` + `insert`).

Note that PR rust-lang#60467 improved this code somewhat; this is a further
improvement.

r? @davidtwco

bors added a commit that referenced this pull request May 22, 2019

Auto merge of #61018 - Centril:rollup-pouzggz, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #60581 (convert custom try macro to `?`)
 - #60963 (Update boxed::Box docs on memory layout)
 - #60973 (Avoid symbol interning in `file_metadata`.)
 - #60982 (Do not fail on child without DefId)
 - #60991 (LocalDecl push returns Local len)
 - #60995 (Add stream_to_parser_with_base_dir)
 - #60998 (static_assert: make use of anonymous constants)
 - #61006 (adjust deprecation date of mem::uninitialized)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request May 22, 2019

Rollup merge of rust-lang#60973 - nnethercote:fix-file_metadata-more,…
… r=michaelwoerister

Avoid symbol interning in `file_metadata`.

This commit changes `created_files` so it uses strings directly as keys,
rather than symbols derived from the strings. This avoids the cost of
having to do the hash table lookups to produce the symbols from the
strings.

The commit also uses `entry` to avoid doing a repeated hash table lookup
(`get` + `insert`).

Note that PR rust-lang#60467 improved this code somewhat; this is a further
improvement.

r? @davidtwco

bors added a commit that referenced this pull request May 22, 2019

Auto merge of #61027 - Centril:rollup-oewauf1, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #59742 (Move `edition` outside the hygiene lock and avoid accessing it)
 - #60581 (convert custom try macro to `?`)
 - #60963 (Update boxed::Box docs on memory layout)
 - #60973 (Avoid symbol interning in `file_metadata`.)
 - #60982 (Do not fail on child without DefId)
 - #60991 (LocalDecl push returns Local len)
 - #60995 (Add stream_to_parser_with_base_dir)
 - #60998 (static_assert: make use of anonymous constants)
 - #61003 (Remove impls for `InternedString`/string equality.)
 - #61006 (adjust deprecation date of mem::uninitialized)

Failed merges:

r? @ghost

@bors bors merged commit c5d9401 into rust-lang:master May 22, 2019

1 of 2 checks passed

homu Testing commit c5d94017569713c12a7e3d260cb4cd0a02b26372 with merge b02dc3ce95855e249764e18415a05555ae4202c5...
Details
Travis CI - Pull Request Build Passed
Details

@nnethercote nnethercote deleted the nnethercote:fix-file_metadata-more branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.