https://pantsbuild.org/ logo
Powered by
a

aloof-angle-91616

01/27/2019, 10:43 PM
@average-vr-56795 i was about to start making everything else in 
Store
use the 
v2::*
protobuf types, including e.g. 
bazel_protos::build::bazel::remote::execution::v2::Directory
. is that a useful cleanup (as long as they're the same bytes), or is that going to break other things if we do it right now?
a

average-vr-56795

01/27/2019, 10:51 PM
We should only do that when we're migrating to use tower for the RPCs too, otherwise we'll need to do a bunch of conversions which aren't free at the RPC boundary
👍 1
Feel free to pick to the tower migration if you want, but IIRC you're also part way through investigating whether we need to revert the half of the tower migration I already did...
a

aloof-angle-91616

01/27/2019, 10:52 PM
hm well i was about to say the conversion hasn't been that hard and being able to specify structs with literal fields is really quite nice
(but so far i have only converted to use 
Action
and 
ActionResult
from the new protos)
the one thing i'm running into right now is that everything in 
<http://remote.rs|remote.rs>
is still using 
bazel_protos::remote_execution::Action
, which is now not what we are using to store things in the 
ActionCache
(which i renamed to 
ActionSerializer
for some reason), so there's one tiny conversion necessary. but i think i can stay out of 
fs
from now on at least
a

average-vr-56795

01/27/2019, 10:57 PM
Conversion is easy code wise, but it has a runtime cost copying bytes around which we shouldn't introduce.
a

aloof-angle-91616

01/27/2019, 10:57 PM
right, that's what you said above, got it
a

average-vr-56795

01/27/2019, 10:58 PM
So one conversion when writing to the action cache is probably fine if it can't be easily avoided, but we shouldn't go further than that :)
a

aloof-angle-91616

01/27/2019, 10:58 PM
ok that clears it up! thank you!
the distinction between "copying bytes to get a local cache key" and "copying bytes to get something to send remotely" is clear enough perf wise
a

average-vr-56795

01/27/2019, 10:59 PM
(but we can probably avoid it at the action cache boundary too, I imagine)
a

aloof-angle-91616

01/27/2019, 11:00 PM
so would it preferable to use 
bazel_protos::build::bazel::remote::execution::v2::Action
everywhere then?
sorry if i'm being obtuse
that mostly requires changing some lines of test code in 
<http://remote.rs|remote.rs>
and would get us the goal of "not changing protos for 
Action
objects at all"
a

average-vr-56795

01/27/2019, 11:02 PM
It would be better for the Store to have methods which accept inconsistent types, than to choose a consistent type and pay a runtime cost
I don't have the code in front of me, but basically, please prioritise, in descend order: 1. Avoid runtime cost 2. Avoid introducing new usages of the old proto types (prefer the ones in gen_for_tower over gen) 3. Aim to have individual structs and traits take and return consistent types
But also, do whatever you're doing, and we can talk about it in code review if there's something that can be improved! :)
a

aloof-angle-91616

01/27/2019, 11:05 PM
ok, i suspected you were e.g. on a phone and appreciate the time to specify this!
what you've described above is aligned with what i was thinking
a

average-vr-56795

01/27/2019, 11:06 PM
Cool, sounds good!
a

aloof-angle-91616

01/27/2019, 11:07 PM
i think we won't need to be introducing new usages of the old types at all, i believe those can be localized strictly to usage in the 
Store
and avoid marshalling between proto types entirely
thanks!
Powered by