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.

Sign up for GitHub

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

Jump to bottom

Implement typechecking for monomorphic "deriving" traits #3837

Closed
wants to merge 1 commit into from
Closed

Implement typechecking for monomorphic "deriving" traits #3837

wants to merge 1 commit into from

Conversation

pcwalton
Copy link
Contributor

No description provided.

@pcwalton
Copy link
Contributor Author

r? @brson

@brson
Copy link
Contributor

brson commented Oct 23, 2012

encoder.rs doesn't distinguish between an impl with no methods and an impl with no body at all, while the parser and pretty-printer make it seem like this distinction is important. Is it?

@brson
Copy link
Contributor

brson commented Oct 23, 2012

It's a shame that the deriving machinery needs to be split across several major passes, in particular trans. Derived impls seemingly shouldn't require any special translation.

@brson
Copy link
Contributor

brson commented Oct 23, 2012

r+

@pcwalton
Copy link
Contributor Author

Would it be better to just indicate derivingness by not implementing a derivable method?

@brson
Copy link
Contributor

brson commented Oct 23, 2012

I wasn't thinking of anything with a language impact, but just like a Implemented/Deriving flag on the AST node instead of an option. It's nbd either way.

@brson
Copy link
Contributor

brson commented Oct 23, 2012

I do understand how the absence (None) of methods means they need to be implemented.

@brson
Copy link
Contributor

brson commented Oct 23, 2012

In fact, I like it better as written now. Yay.

@brson brson closed this Oct 23, 2012
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Aug 30, 2024
@JoJoDeveloping
Make TB tree traversal bottom-up
2765444 
In preparation for rust-lang#3837, the tree traversal needs to be made bottom-up,
because the current top-down tree traversal, coupled with that PR's
changes to the garbage collector, can introduce non-deterministic error
messages if the GC removes a parent tag of the accessed tag that would
have triggered the error first.

This is a breaking change for the diagnostics emitted by TB. The
implemented semantics stay the same.
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Aug 30, 2024
@bors
Auto merge of rust-lang#3843 - JoJoDeveloping:tb-bottom-up-iteration,…
4318bfe 
… r=RalfJung

Make TB tree traversal bottom-up

In preparation for rust-lang#3837, the tree traversal needs to be made bottom-up, because the current top-down tree traversal, coupled with that PR's changes to the garbage collector, can introduce non-deterministic error messages if the GC removes a parent tag of the accessed tag that would have triggered the error first.

This is a breaking change for the diagnostics emitted by TB. The implemented semantics stay the same.
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Aug 30, 2024
@bors
Auto merge of rust-lang#3837 - JoJoDeveloping:tb-compacting-provenanc…
9ad0f65 
…e-gc, r=RalfJung

Make Tree Borrows Provenance GC compact the tree

Follow-up on rust-lang#3833 and rust-lang#3835. In these PRs, the TB GC was fixed to no longer cause a stack overflow. One test that motivated it was the test `fill::horizontal_line` in [`tiny-skia`](https://github.com/RazrFalcon/tiny-skia). But not causing stack overflows was not a large improvents, since it did not fix the fundamental issue: The tree was too large. The test now ran, but it required gigabytes of memory and hours of time (only for it to be OOM-killed 🤬), whereas it finishes within 24 seconds in Stacked Borrows. With this merged, it finishes in about 40 seconds under TB.

The problem in that test was that it used [`slice::chunked`](https://doc.rust-lang.org/std/primitive.slice.html#method.chunks) to iterate a slice in chunks. That iterator is written to reborrow at each call to `next`, which creates a linear tree with a bunch of intermediary nodes, which also fragments the `RangeMap` for that allocation.

The solution is to now compact the tree, so that these interior nodes are removed. Care is taken to not remove nodes that are protected, or that otherwise restrict their children.

I am currently only 99% sure that this is sound, and I do also think that this could compact even more. So `@Vanille-N` please also have a look at whether I got the compacting logic right.

For a more visual comparison, [here is a gist](https://gist.github.com/JoJoDeveloping/ae4a7f7c29335a4c233ef42d2f267b01) of what the tree looks like at one point during that test, with and without compacting.

This new GC requires a different iteration order during accesses (since the current one can make the error messages non-deterministic), so it is rebased on top of rust-lang#3843 and requires that PR to be merged first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pcwalton @brson