Why is peek-poke supporting Euclid and not the other way?

[kvark]

It seems to me that peek-poke is a lower level primitive here that adds extra properties to Euclid types. Why isn't there an optional dependency to peek-poke from here that just adds a bunch of auto-derives? I'm surprised to see https://github.com/djg/peek-poke/blob/728591c140ead9a6b646a27eaad29e5136fb96b8/src/euclid.rs 👀 cc @djg

[nox]

peek-poke isn't published, so euclid can't depend on it, for starters. And I think @djg's clone isn't the peek-poke used in WR.

We should probably move peek-poke to its own repo under the servo org, publish it, and then start reversing the dependencies afterwards.

[kvark]

Thanks @nox! To be fair, the work originally could have been done in a euclid fork that depends on peek-poke github repo. This would be less awkward. Anyhow, filed https://github.com/djg/peek-poke/issues/7

[nox]

To be fair, the work originally could have been done in a euclid fork that depends on peek-poke github repo. This would be less awkward.

Then we would have needed to use the fork from Servo. :)

[djg]

Why would Euclid care about peek-poke? Ie. I don’t buy your argument that peek-poke is lower level.

In separation of concerns I don’t see why Euclid should know how to serialise/deserialise when that’s the job of peek-poke.

On Thu, 9 Jan 2020 at 2:40 am, Anthony Ramine notifications@github.com wrote:

To be fair, the work originally could have been done in a euclid fork that depends on peek-poke github repo. This would be less awkward.

Then we would have needed to use the fork from Servo. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/servo/euclid/issues/394?email_source=notifications&email_token=AABGDLMQR76CANDG3DK4PPTQ4X6X3A5CNFSM4KEK5MUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEINF45I#issuecomment-572153461, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGDLOOBBJBBMZ5HC62PFDQ4X6X3ANCNFSM4KEK5MUA .

-- Dan Glastonbury, Dan dot Glastonbury at gmail dot com `Pour encourjay lays ortras'

[kvark]

Following this logic, Serde should have an optional dependency on Euclid so that it can serialize its types :/

[nical]

It wouldn't make sense for euclid to maintain optional dependencies to every single serialization crate. Considering how niche peek-poke is (basically tailoring to the specific needs of WebRender), it doesn't strike me as a bad thing for the dependency to go in that direction, and it avoids having an extra feature flag in euclid.

It comes down to which principle is more important to you (low-level/high-level vs general/niche). I can totally understand that you feel stronger about the former, the latter also has its merits.

That said, if we feel that peek-poke is stable-ish I wouldn't mind having euclid depend on peek-poke either, but please don't fork euclid, there's too many inter-connected crates that depend on it, it will make a mess and the direction of the peek-poke dependency is not worth that kind of mess.

[kvark]

@nical there are way more potential things that need to be serialized than there are serialization crates. Not sure what "every single serialization crate" you mean here, other than "serde" + "peek-poke". Is there any other serialization crate that has optional dependencies on the stuff it serializes?

[nical]

I poorly explained my point about depending on every serialization crates. It was meant to illustrate that a component being more generic doesn't automatically mean the less general code should depend on it, but was not a good analogy.

The main point is that there are several guiding principles you can adhere to when deciding which way a dependency go. One is to say that serialization is a generic system and creators of new types should be the one to opt into that system (the point you are making which is a good one), another being to consider that a if a piece (such as peek-poke) is a bit niche (focused on webrender's needs) it should be the piece that depends on more universally used components (like euclid which has many users outside of webrender).

I don't disagree with the first principle, neither do I disagree with the second one (and in this case they contradict each other).

Taking a step back from principles, from a more practical point of view, it's rather painless to publish breaking changes to peek-poke right now but it's quite painful to publish breaking changes to euclid. If we change the dependency, making breaking changes to peek-poke will become painful, we have to keep that in mind. It seems to me that the current situation lacks in elegance but does not bring any practical issues.

I would lean towards not having euclid depend on webrender-specific things, in particular if they are moving parts. But if peek-poke is not a temporary workaround to a serde/bincode performance problem and if the dependency is stable and lightweight, then I'm fine with flipping the direction of the dependency.

[kvark]

@nical we spoke about this distinction early today with @jrmuizel . Basically, the conclusion I came to is: if we want to make peek-poke to be WR-specific, it needs to be just a module in WR. Then all the questions about what depends on what, and how to publish stuff, are naturally resolved. As soon as peek-poke becomes a standalone crate, which has already happened, it needs to behave like a crate and not drag the high level dependencies unnecessarily.

[nical]

It can still be a crate if there is merit to it being a separate compilation unit, just like webrender_api and gfx_window_dxgi are crates, as long as being specific to webrender is documented (the current crates.io description mentions webrender).

[nox]

Why close this?

[nical]

Closed because the conclusion was that peek-poke is a webrender-specific work around for serialization performance, and that it isn't intended for use outside of webrender. As such it is best to make it depend on whatever webrender needs rather than the opposite. See also https://github.com/djg/peek-poke/issues/7#issuecomment-575645799

[kvark]

I see it differently. It was suggested that we put a disclaimer that the project isn't intended on anything but webrender in the description for publishing reasons, so that others don't get too excited to use it as well. This is the message to outside world. It doesn't mean that internally we have to be strictly using it for webrender only forever. This dependency still doesn't make sense, as I explained in https://github.com/servo/euclid/issues/394#issuecomment-576369895

[nical]

Ok well, let's talk about it next week.