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 after recent rust-nightly changes. #19

Merged
merged 2 commits into from
Aug 10, 2020
Merged

Update after recent rust-nightly changes. #19

merged 2 commits into from
Aug 10, 2020

Conversation

ltratt
Copy link
Member

@ltratt ltratt commented Aug 6, 2020

There are two PRs which affect us:

rust-lang/rust#74850
rust-lang/rust#75152

Henceforth, we should probably be keeping a close eye on and contributing to:

https://github.com/rust-lang/wg-allocators/issues

@jacob-hughes
Copy link
Contributor

Good idea. This stuff is moving quite fast and breakages tend to happen a lot in the alloc API.

bors r+

bors bot added a commit that referenced this pull request Aug 7, 2020
19: Update after recent rust-nightly changes. r=jacob-hughes a=ltratt

There are two PRs which affect us:

  rust-lang/rust#74850
  rust-lang/rust#75152

Henceforth, we should probably be keeping a close eye on and contributing to:

  https://github.com/rust-lang/wg-allocators/issues

Co-authored-by: Laurence Tratt <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 7, 2020

Build failed:

@ltratt
Copy link
Member Author

ltratt commented Aug 7, 2020

Sausages. We've been hoist by rustfmt's petard: the last version of rustc with a working rustfmt predates this change :/

Any thoughts?

@ltratt
Copy link
Member Author

ltratt commented Aug 7, 2020

Thinking about this, I think that we have to wait to merge this PR, because we have dependent repos that expect a working rustfmt.

What that means is that, when building locally, one needs to use a specific version of rust (2020-07-27 to be specific) until rustfmt builds again and we can merge this. What a mess!

@ltratt
Copy link
Member Author

ltratt commented Aug 9, 2020

bors try

bors bot added a commit that referenced this pull request Aug 9, 2020
@bors
Copy link
Contributor

bors bot commented Aug 9, 2020

try

Build failed:

@ltratt
Copy link
Member Author

ltratt commented Aug 9, 2020

bors try

bors bot added a commit that referenced this pull request Aug 9, 2020
@bors
Copy link
Contributor

bors bot commented Aug 9, 2020

try

Build failed:

@ltratt
Copy link
Member Author

ltratt commented Aug 9, 2020

bors try

bors bot added a commit that referenced this pull request Aug 9, 2020
@bors
Copy link
Contributor

bors bot commented Aug 9, 2020

try

Build failed:

@ltratt
Copy link
Member Author

ltratt commented Aug 9, 2020

bors try

bors bot added a commit that referenced this pull request Aug 9, 2020
@bors
Copy link
Contributor

bors bot commented Aug 9, 2020

try

Build failed:

@ltratt
Copy link
Member Author

ltratt commented Aug 9, 2020

bors try

bors bot added a commit that referenced this pull request Aug 9, 2020
@bors
Copy link
Contributor

bors bot commented Aug 9, 2020

try

Build failed:

@ltratt
Copy link
Member Author

ltratt commented Aug 9, 2020

bors try

bors bot added a commit that referenced this pull request Aug 9, 2020
@bors
Copy link
Contributor

bors bot commented Aug 9, 2020

try

Build succeeded:

@ltratt
Copy link
Member Author

ltratt commented Aug 9, 2020

OK for me to squash?

bors bot added a commit to softdevteam/yksom that referenced this pull request Aug 10, 2020
175: Split `Frame::new` into two. r=ptersilie a=ltratt

This function is one of the most frequently called in yksom, so the fact that it serves two different masters (methods and blocks) is less than ideal (at the very least it introduces a branch). This commit splits the function into two, one for methods and one for blocks.

To some extent this is a bit pointless in that the major overhead of `Frame::new` is the malloc of a `Vec`, but one day that cost will be much less, so this change seems like a useful one, if perhaps a bit early!

This can be reviewed but won't be mergeable until softdevteam/libgc#19 is merged.

176: Lazily store the number of Unicode characters a string contains. r=ptersilie a=ltratt

Calculating the number of Unicode characters in a string is an O(n) operation. Previously everytime `String_::length` was called we iterated over the underlying string to discover how many Unicode characters it contains.

This commit adds a field to SOM strings that stores its number of Unicode characters. Since not all strings will be queried for this property, we lazily cache this property the first time that `length` is called on a `String_`. We use `usize::max` as our marker as that means the marker is "safe" in all cases.

On the JSON benchmark, with GC off, this speeds things up by about 80%.

Needs softdevteam/libgc#19 to be merged first.

Co-authored-by: Laurence Tratt <[email protected]>
bors bot added a commit to softdevteam/yksom that referenced this pull request Aug 10, 2020
177: Remove calls to tobj() when we know we must have a GCBOX. r=ptersilie a=ltratt

Needs softdevteam/libgc#19 to be merged first.

At the moment a `Val` can only be `INT` or `GCBOX`, so in cases where we've checked to see if `self` is an `INT`, the only remaining case is `GCBOX`. The locations changed in this commit implicitly checked that `self` was a `GCBOX`
twice: by using unsafe `gcbox_to_tobj` function, we can remove the second (pointless) check. This is a just about measurable improvement on the JSON benchmark at about 1%.

Co-authored-by: Laurence Tratt <[email protected]>
@ltratt
Copy link
Member Author

ltratt commented Aug 10, 2020

bors try

bors bot added a commit that referenced this pull request Aug 10, 2020
@bors
Copy link
Contributor

bors bot commented Aug 10, 2020

try

Build succeeded:

@ltratt
Copy link
Member Author

ltratt commented Aug 10, 2020

bors try

bors bot added a commit that referenced this pull request Aug 10, 2020
@bors
Copy link
Contributor

bors bot commented Aug 10, 2020

try

Build succeeded:

.buildbot.sh Outdated
# Install no toolchain initially to ensure toolchain rollback.
sh rustup.sh --default-host x86_64-unknown-linux-gnu --default-toolchain none -y --no-modify-path

sh rustup.sh --default-host x86_64-unknown-linux-gnu --default-toolchain nightly -y --no-modify-path
Copy link
Member

Choose a reason for hiding this comment

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

can we select the minimal profile here? If so we can kill the following invocation of rustup?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can. Fixed in 0a47536.

@ltratt
Copy link
Member Author

ltratt commented Aug 10, 2020

bors try

bors bot added a commit that referenced this pull request Aug 10, 2020
@bors
Copy link
Contributor

bors bot commented Aug 10, 2020

try

Build succeeded:

@ltratt
Copy link
Member Author

ltratt commented Aug 10, 2020

I found the number of args was getting out of hand, so I've done a reformat. But otherwise I think this might be ready for squashing?

@vext01
Copy link
Member

vext01 commented Aug 10, 2020

Great!

Can't comment on the other changes, but looks like it's pre-reviewed by Jake.

Please squash.

@ltratt
Copy link
Member Author

ltratt commented Aug 10, 2020

Squashed.

@vext01
Copy link
Member

vext01 commented Aug 10, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 10, 2020

Build succeeded:

@bors bors bot merged commit d20a693 into softdevteam:master Aug 10, 2020
@ltratt ltratt deleted the update_after_rust_lib_changes branch August 10, 2020 09:23
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.

3 participants