| *** Reagan5616 has joined #buildstream | 00:33 | |
| *** Reagan5616 has quit IRC | 00:35 | |
| *** tristan has quit IRC | 06:15 | |
| *** tristan has joined #buildstream | 06:28 | |
| *** tristan has quit IRC | 07:00 | |
| *** tristan has joined #buildstream | 07:01 | |
| *** ChanServ sets mode: +o tristan | 07:21 | |
| tristan | juergbi, looking at doing the `link` element thing now... what I'm thinking is that from the loader perspective, I need to resolve links at any of their usage points; which is to say: at the moment we call get_loader() we need to discover a link and resolve it immediately, and the same goes for when we traverse dependencies in loader._load_file() | 07:21 |
|---|---|---|
| tristan | Intuitively one might think we could do this at a single location, but the various stages of load states appears to prevent this (e.g. at first they are nodes, then MetaElements, then Elements) | 07:22 |
| tristan | We cannot just do a substitution and load multiple MetaElements pointing to the same file | 07:22 |
| juergbi | tristan: can't we actually reuse the same MetaElement? | 07:23 |
| tristan | Hmm | 07:23 |
| tristan | Well yes that would be the result | 07:23 |
| juergbi | i.e., resolve early once and then the link is irrelevant | 07:23 |
| tristan | We'd still have to special case links along the way, I think | 07:23 |
| juergbi | outside the loader? | 07:24 |
| tristan | No | 07:24 |
| juergbi | ok and in the loader probably not in more places than we special case junction | 07:24 |
| tristan | But for both cases, at the time of asking for a junction (evaluating the junction.bst), and before getting a file to load (when looping over dependencies and calling _load_file()) | 07:25 |
| juergbi | i.e., we have two special case instead of one but in the same places, right? | 07:25 |
| tristan | Right | 07:25 |
| tristan | I think there is an overhead too here to using a link if I'm not mistaken | 07:25 |
| tristan | It might pay off to cache the link resolutions | 07:25 |
| tristan | Otherwise we load the file and resolve every time anything *refers* to a link | 07:25 |
| juergbi | that's true, we should try to avoid that | 07:26 |
| juergbi | especially as link .bst files might have conditionals, i.e., they aren't always just two yaml nodes | 07:26 |
| tristan | Indeed | 07:26 |
| juergbi | but I guess that also holds for junction links | 07:26 |
| juergbi | i.e., not that different from supporting links only for junctions, afaict | 07:27 |
| tristan | Ummm, compared to the current junction "targets" implementation you mean ? | 07:28 |
| tristan | That's certainly different I think, it's resolved only once | 07:28 |
| tristan | So, I suppose Loader would gain a new _links dictionary to store resolved link Elements (not MetaElements) | 07:28 |
| tristan | The `link` Elements would then be used to redirect things at load time | 07:29 |
| tristan | There will be an additional node.get_str('kind') == 'link' check for every dependency | 07:29 |
| juergbi | right, at the moment we store a dict from junction name to loaders and we need a new dict from link name to target | 07:30 |
| tristan | Right perhaps no need to keep the loaded Element in context, loading the Element is just a formality of accessing the loaded target, and exercising assertions like BST_FORBID_SOURCES etc | 07:31 |
| tristan | either way, we're only interested in caching the target | 07:31 |
| juergbi | tristan: maybe add a target attribute to LoadElement? | 07:32 |
| tristan | Then still, for every access to any element, there would still be a preemptive check to try to fetch the target from the _link cache (even if it's been resolved once) | 07:32 |
| juergbi | as we already have a dict for that, avoiding an extra lookup | 07:32 |
| tristan | I'm not sure I follow | 07:32 |
| juergbi | tristan: in _load_file() we currently do a lookup for every dependency in self._elements | 07:34 |
| juergbi | these are LoadElement instances | 07:34 |
| tristan | So we have an `_elements` cache of LoadElements, you would have the loaded `link` cached as a LoadElement with a target member... not sure where it's leading | 07:35 |
| * tristan doesnt think he likes the data model this way either | 07:35 | |
| tristan | Right | 07:35 |
| juergbi | maybe when resolving a link, we could add an extra entry to self._elements where we directly point to the target LoadElement | 07:35 |
| tristan | Mhm | 07:35 |
| juergbi | so the next time the link is specified as dependency, we directly get the target | 07:35 |
| tristan | Ah | 07:35 |
| juergbi | it's been a while since looking at loader code, so I might be overlooking something | 07:35 |
| tristan | And, same for `_loaders` ? | 07:36 |
| tristan | I do see where you're going now | 07:36 |
| juergbi | yes, I think so. _loaders for junction links and _elements for regular element links | 07:36 |
| tristan | One hash lookup + member access vs 2 hash lookups | 07:36 |
| juergbi | and it might even be less code | 07:36 |
| tristan | Ok lets try and see :) | 07:37 |
| juergbi | the performance difference might not actually be significant compared to YAML processing | 07:37 |
| juergbi | so code legibility is probably higher priority | 07:37 |
| juergbi | but if it's fast and simple code, even better :) | 07:38 |
| tristan | Yeah, the loader is not the most legible of territories | 07:38 |
| tristan | interestingly, junctions are also cached in _elements as LoadElements | 08:11 |
| tristan | Hmmm, this is working nicely | 09:32 |
| tristan | Only one test written, I'll have to add a bunch of tests, still have to implement for elements, and then remove the whole `target` junction feature separately in the same branch | 09:33 |
| tristan | But looks like it will be more legible as a result | 09:33 |
| tristan | whoa that loop in _load_file() is really a mind bender :-S | 11:29 |
| tristan | So when we load a dependency across a junction, it's okay to recurse, but not when loading dependencies within the same project ? | 11:35 |
| * tristan doesnt understand 17144d84c2 ... why can the exact same thing not be achieved without adding this extra _NO_PROGRESS variable, which I think None is a perfectly good value for ? | 12:12 | |
| tristan | newsflash: `if foo:` truthy statements fail pylint when `foo` is defined in cython as readonly; pylint says this is "Using a conditional statement with a constant value (using-constant-test)" | 13:00 |
| tristan | this gets swept under the rug when changing it to `if foo is not None:`, which might be an explanation for the suspiciously growing number of `is not None` statements in our code | 13:01 |
| *** Valentina2055 has joined #buildstream | 13:21 | |
| *** Valentina2055 has quit IRC | 13:22 | |
| *** Gianna6275 has joined #buildstream | 13:56 | |
| *** slaf has quit IRC | 13:56 | |
| *** slaf has joined #buildstream | 13:57 | |
| *** Gianna6275 has quit IRC | 13:57 | |
| *** jswagner has quit IRC | 13:58 | |
| *** tristan has quit IRC | 14:43 | |
| *** Genesis5882 has joined #buildstream | 14:50 | |
| *** Genesis5882 has quit IRC | 14:52 | |
| *** tristan has joined #buildstream | 15:17 | |
| *** tristan has quit IRC | 15:38 | |
| *** philn has quit IRC | 15:38 | |
| *** phildawson has quit IRC | 15:38 | |
| *** traveltissues has quit IRC | 15:38 | |
| *** chipb has quit IRC | 15:38 | |
| *** SotK has quit IRC | 15:38 | |
| *** lantw44 has quit IRC | 15:38 | |
| *** Trevinho has quit IRC | 15:38 | |
| *** chipb has joined #buildstream | 15:38 | |
| *** tristan has joined #buildstream | 15:38 | |
| *** philn has joined #buildstream | 15:39 | |
| *** traveltissues has joined #buildstream | 15:39 | |
| *** phildawson has joined #buildstream | 15:39 | |
| *** Trevinho has joined #buildstream | 15:40 | |
| *** SotK has joined #buildstream | 15:40 | |
| *** phildawson has quit IRC | 19:23 | |
| *** phildawson has joined #buildstream | 19:23 | |
| *** Ava4991 has joined #buildstream | 21:12 | |
| *** Ava4991 has quit IRC | 21:13 | |
| *** phildawson has quit IRC | 21:18 | |
| *** jswagner has joined #buildstream | 22:12 | |
| *** jswagner has joined #buildstream | 22:13 | |
| *** jswagner has joined #buildstream | 22:14 | |
| *** Ella3089 has joined #buildstream | 23:14 | |
| *** Ella3089 has quit IRC | 23:16 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!