Stream: t-cargo/PubGrub

Topic: arena


view this post on Zulip Matthieu Pizenberg (Feb 26 2021 at 17:37):

@Eh2406 in the arena module why are you defining the clone, copy, eq, ... traits instead of deriving them? is this because of the phantom data? By the way it's the first time I see use of it so I'm discovering it a bit while reading the code

view this post on Zulip Eh2406 (Feb 26 2021 at 17:40):

IDK, try removing the impls and adding the derive. If it works, and the test pass, then I did it by accident.

view this post on Zulip Eh2406 (Feb 26 2021 at 17:42):

phantom data

is just a way to tell the type system that I am using a T, while telling the code generator that I am not.
This lets us keep track of the fact that this u32 does not have the same type as one from a different arena. But at runtime it is just a u32.

view this post on Zulip Matthieu Pizenberg (Feb 26 2021 at 17:47):

Eh2406 said:

IDK, try removing the impls and adding the derive. If it works, and the test pass, then I did it by accident.

Indeed, deriving some properties requires additional bounds on T that we actually do not need because it's phantom, so that's why you manually defined those. Some people link to this thread regarding similar cases with derive: https://github.com/rust-lang/rust/issues/26925

view this post on Zulip Eh2406 (Feb 26 2021 at 17:54):

That makes sense. Can you add a comment explaining that?

view this post on Zulip Eh2406 (Feb 26 2021 at 17:54):

Or do you want me to?

view this post on Zulip Matthieu Pizenberg (Feb 26 2021 at 17:55):

Yep just did that

view this post on Zulip Matthieu Pizenberg (Feb 26 2021 at 17:55):

I'll do a few commits for docs of things I come accross while reading

view this post on Zulip Matthieu Pizenberg (Feb 26 2021 at 18:28):

I'll write few things down here while reading. Don't feel like you need to answer those now. It's just so that you know what things are going through my head while reading. And I might even find answers to some of those on my own while continuing reading. Just doing that so that I don't post a ton of things all at once. But I'll keep those notes and organize a summary of my questions at the end.

view this post on Zulip Matthieu Pizenberg (Feb 26 2021 at 18:28):

[ ] std::any::type_name wasn't that the one with a security issue at some point?
[ ] Should we start bigger for Arena::new()?
[ ] why using id.into_raw() for Derived.shared_id? It is used in the reporting code, but we actually do not need that to be a usize per-se. So going forward, we should refactor the reporting code to use the Id type instead and not having to expose an into_raw method.

view this post on Zulip Matthieu Pizenberg (Feb 26 2021 at 19:36):

[ ] do we actually need Incompatibility::from_dependencies? It is allocating a vec, and it seems it's only used to do two things, (1) iterate over newly created incompats in solver::resolve and add them to the incompatibility store (arena), and (2) add the version to the partial solution if the dependencies are not problematic. (2) needs references to those incompatibilities, which could be done via the Arena if it implemented something like slices or iterators starting at a given Id and of a given size. (don't know if that's possible)

view this post on Zulip Eh2406 (Feb 26 2021 at 20:18):

std::any::type_name wasn't that the one with a security issue at some point?

I don't think so, but now I can't find it. I think the one with the issues let you take an ID and turn it into a T.
https://blog.rust-lang.org/2019/05/13/Security-advisory.html

view this post on Zulip Eh2406 (Feb 26 2021 at 20:23):

Should we start bigger for Arena::new()?

worth experimenting! Not worth holding things up for.

view this post on Zulip Eh2406 (Feb 26 2021 at 20:26):

So going forward, we should refactor the reporting code to use the Id type instead and not having to expose an into_raw method.

Yes! But that is a braking change and so far we have not made any on dev. So let's look into that after a 2.1

view this post on Zulip Eh2406 (Feb 26 2021 at 20:29):

do we actually need Incompatibility::from_dependencies

Did not think of this! We should definitely experiment!

view this post on Zulip Matthieu Pizenberg (Feb 27 2021 at 16:24):

Eh2406 said:

So going forward, we should refactor the reporting code to use the Id type instead and not having to expose an into_raw method.

Yes! But that is a braking change and so far we have not made any on dev. So let's look into that after a 2.1

Very true!

view this post on Zulip Matthieu Pizenberg (Feb 27 2021 at 16:35):

[ ] merge_into(id: Id<Self>, incompats: &mut Vec<Id<Self>>) method becomes an associated function
[ ] build_derivation_tree method also becomes an associated function, are they both still located in the right module?

view this post on Zulip Matthieu Pizenberg (Feb 27 2021 at 16:35):

[ ] I like the simpler State.add_incompatibility!

view this post on Zulip Eh2406 (Feb 27 2021 at 17:12):

merge_into

Should just be inlined. Except then what to do with the comment.
So until we have a plan for how to impl that comment, I don't think there will be a "good place" for it.

view this post on Zulip Matthieu Pizenberg (Feb 27 2021 at 17:13):

[ ] does conflict_resolution need the id? or can pass the &Incompat?

    -> it returns the id in the end, is it needed?
    -> it calls build_derivation_tree(id) (same question, need id?)
       -> it calls Incompatibility::build_derivation_tree which needs it since it has to know if it's a shared id
       BTW seems like build_derivation_tree would be better located directly in the core module.
    -> it calls self.backtrack(id) (same question, need id?)
        -> it calls Incompat::merge_into, which needs the id since it merges into a Vec of Ids
    -> it calls Incompat::prior_cause, which I guess def need the id

[ ] same question for self.partial_solution.add_derivation?

view this post on Zulip Matthieu Pizenberg (Feb 27 2021 at 22:34):

@Eh2406 with all the performance work that (mostly you) we have done since 2.0 there is a huge boost. I just relaunched the code solving dependencies of all versions of all elm packages (same from last time, I didn't updated it with the new packages since then). It went from 900ms to 400ms in the latest version


Last updated: Oct 21 2021 at 20:47 UTC