| *** tristan has quit IRC | 07:18 | |
| *** benschubert has joined #buildstream | 07:54 | |
| *** phildawson has joined #buildstream | 08:21 | |
| *** santi has joined #buildstream | 08:41 | |
| *** tomaz has joined #buildstream | 09:20 | |
| *** tristan has joined #buildstream | 09:33 | |
| *** ChanServ sets mode: +o tristan | 09:33 | |
| *** tristan has quit IRC | 09:41 | |
| *** tristan has joined #buildstream | 10:49 | |
| *** ChanServ sets mode: +o tristan | 10:49 | |
| douglaswinship | is there a good way to find out how large a file is, which you're in a "bst shell" shell? I tried running du, and it said everything was 0 Bytes. | 10:55 |
|---|---|---|
| douglaswinship | I'm guessing that's got something to do with the "virtual" part of the virtual directory system. | 10:56 |
| juergbi | douglaswinship: have you already tried 'ls -l' or 'du --apparent-size'? | 10:57 |
| juergbi | I assume this is on master with buildbox-fuse | 10:58 |
| juergbi | we may be able to more closely emulate a block device to fix `du` | 10:58 |
| douglaswinship | juergbi: aha, --apparent-size works. Thanks! | 10:59 |
| tristan | benschubert, any further thoughts on optimizing _variables.pyx after my latest comment ? (A) do you think the added memory is a concern ? (B) do you think it worthwhile to reuse cached `Variables` collections on elements where they would be resolved identically ? | 11:12 |
| tristan | I also thought of hashing Values based on their dependency ValueClasses and reusing there, but that would certainly be more costly than just resolving them | 11:13 |
| tristan | There may also be something to do with provenance identity comparisons | 11:13 |
| tristan | if two Elements have a collection of loaded Values which are essentially equal, and every named Variable has the same provenances, we can just reuse a fully resolved Variables collection | 11:14 |
| * tristan has tried using python deque() instead of cython list, but that adds overhead in valentind's test sample data | 11:15 | |
| benschubert | tristan: | 11:17 |
| benschubert | A) It is, but I think we can live with it if the code is faster and easier to maintain | 11:17 |
| benschubert | 2) I don't think it would be easy to do that nicely | 11:17 |
| benschubert | What I'm wondering is, can we do it in two passes? One as efficient as possible, which detects an error state but can't give a nice error, and then another one we fallback on on error? | 11:17 |
| benschubert | I might be able to look at it more in details this afternoon if needed | 11:17 |
| tristan | benschubert, For the memory overhead, what we really gain is provenance of all values in a form that is easy to reach, so actually it will be very easy to do the whole cyclic variable report as is | 11:19 |
| benschubert | So it's useless apart for the report? | 11:20 |
| tristan | There may be a straight forward way to make that go away | 11:20 |
| benschubert | Could we easily do two passes then? | 11:20 |
| tristan | I.e. have it around in temporary objects only there for the duration of the resolution of variables | 11:20 |
| tristan | it's not a whole provenance object, just an address to it | 11:20 |
| tristan | I'm trying to think of how we can get similar performance without complicating the code with a second pass :-S | 11:21 |
| tristan | I mean, we can of course double the code | 11:21 |
| valentind | Is not deque using linked lists? Is not it only better than vectors when doing multi threaded operations? | 11:22 |
| tristan | I was under the impression that deque had efficient pop() due to saving a pointer to the last element | 11:22 |
| valentind | The O(1) insertion does not really apply anymore on modern hardware. | 11:22 |
| tristan | whereas I think of a list having similar properties to a single-linked list | 11:22 |
| valentind | I thought python "list" where vectors. | 11:23 |
| tristan | But I don't know the underlying properties of list | 11:23 |
| tristan | Arrays ? I don't think so | 11:23 |
| tristan | Oh hi valentind | 11:23 |
| tristan | I see that you are also not benschubert :) | 11:23 |
| * tristan continues a conversation and notices that the nick of who he is talking to has changed ;-) | 11:23 | |
| valentind | There are been papers showing vectors were much better than linked list. | 11:24 |
| benschubert | lists in python have a hard time changing size (at 4,7, 13,....) as they include a copy of existing data | 11:24 |
| valentind | I do not think linked lists are used much anymore | 11:24 |
| benschubert | so if you are not needing indexing + appending/poping a lot, a deque is what you want | 11:24 |
| tristan | Yeah | 11:24 |
| tristan | But using python deque in cython is a big overhead | 11:24 |
| benschubert | because cython doesn't know how to access the C interface directly, and thus calls to python | 11:25 |
| tristan | And using deque `from libcpp.deque cimport deque` is undocumented and impossible to achieve :) | 11:25 |
| benschubert | https://wiki.python.org/moin/TimeComplexity is an amazing page when needing stuff like that | 11:25 |
| tristan | it complains that it wants the types of the queue elements to be strongly typed | 11:25 |
| benschubert | tristan: are you sure about deque: https://cython.readthedocs.io/en/latest/src/userguide/wrapping_CPlusPlus.html#standard-library ? | 11:26 |
| tristan | so it should be something like `cdef deque[Tuple(str, ProvenanceInformation, set)]` | 11:26 |
| valentind | https://www.youtube.com/watch?v=TJHgp1ugKGM&feature=youtu.be&t=2948 | 11:26 |
| benschubert | valentind: that's C++ | 11:27 |
| valentind | That is the same CPU. | 11:27 |
| tristan | benschubert, I found something like that (probably the same thing rendered in pdf in ugly googledocs), but not seeing how to use deque() from there really | 11:27 |
| tristan | benschubert, but looking at the vector for instance, notice `cdef vector[int] vect` | 11:28 |
| tristan | So if you want a collection of `int`, which can be defined as `cdef deque[int] variable_name`, that works | 11:28 |
| benschubert | tristan: you could deque[Tuple] | 11:28 |
| benschubert | and then when you pop you cast the tuple | 11:29 |
| tristan | Hmmm, lets try that, where do I import `Tuple` from ? | 11:29 |
| benschubert | because tuple is actually a PyOBject pointer to somewhere | 11:29 |
| tristan | I believe I tried deque[tuple] | 11:29 |
| tristan | And the only `Tuple` I've seen, is from the mypy type hinting stuff | 11:30 |
| tristan | So I felt reluctant to expect that it would be anything meaningful other than useful for doing static type checking | 11:30 |
| benschubert | Seems like we cna either do: | 11:31 |
| benschubert | - a struct | 11:31 |
| benschubert | - a std::pair | 11:31 |
| benschubert | I'd be reluctant on the `std::pair`, as we've managed to avoid C++ until now | 11:31 |
| tristan | benschubert, `from typing import Tuple` <-- Think this is the right `Tuple` ? | 11:31 |
| benschubert | that's typing info, cython can't do anything with it | 11:32 |
| tristan | Yeah | 11:32 |
| tristan | Any idea what `deque[Tuple]` is ? which Tuple from where ? | 11:33 |
| benschubert | where do you see that? | 11:34 |
| * tristan would be happy to just understand how to use `(void *)` | 11:34 | |
| tristan | <benschubert> tristan: you could deque[Tuple] | 11:34 |
| tristan | benschubert, I see that there ^^^^ | 11:34 |
| tristan | And wonder... what is Tuple ? | 11:34 |
| benschubert | ah deque[tuple] sorry | 11:34 |
| tristan | I don't think that works, I'll try again | 11:34 |
| tristan | I think deque[Value] also didnt work (for a cython defined Value object in the same file) | 11:35 |
| benschubert | but would `deque` alone not work? | 11:35 |
| benschubert | actually I think you want `deque` only | 11:35 |
| benschubert | and then cast your return | 11:35 |
| benschubert | valentind: interesting talk, would you have such benchmarks with python? | 11:36 |
| tristan | benschubert, forgive my unknowing of cython, what might that look like ? | 11:39 |
| tristan | If I do `cdef deque pending = deque()`, then the first `deque` says: 'deque' is not a type identifier | 11:39 |
| tristan | maybe it's just a function ? | 11:39 |
| benschubert | oh wait, the page I linked in Cython is about C++ | 11:40 |
| tristan | yeah, I did `from libcpp.deque import deque` since that's all I could find about it, ever | 11:41 |
| tristan | sorry, s/import/cimport again | 11:41 |
| benschubert | oh well | 11:41 |
| benschubert | and a simple deque without typing was way slower right? | 11:42 |
| tristan | wait a sec, I might have something | 11:42 |
| tristan | I actually again did import instead of cimport, and for some reason the parser didnt explode at that line | 11:42 |
| benschubert | because it's valid :) | 11:42 |
| tristan | Oddly...it's now telling me: src/buildstream/_variables.pyx:239:66: Object of type 'deque[T]' has no attribute 'pop' | 11:43 |
| valentind | benschubert, I do not have. But that would be interesting to measure it. | 11:44 |
| benschubert | tristan: can you push your code somewhere I'll look into it after lunch | 11:44 |
| tristan | So `cdef deque pending = deque()` works, and pending.append((a, b, c)) works | 11:45 |
| tristan | but pop is not valid | 11:45 |
| tristan | if only I had like a strategy for investigating how this is supposed to work, like knowing what C header files I should be looking at, hmmm | 11:46 |
| tristan | But seems quite involving for something which only *might* shave off some cycles | 11:46 |
| *** mohan43u has quit IRC | 12:07 | |
| *** mohan43u has joined #buildstream | 12:07 | |
| benschubert | tristan: back, did you find something/ give up? | 12:23 |
| benschubert | I agree that it might not be the most worth optimization | 12:23 |
| benschubert | we probably can do much better | 12:24 |
| benschubert | actually the split between resolving and getting the error was made especially because of the slowness | 12:24 |
| benschubert | so I think it would be hard to do without the split | 12:24 |
| tristan | Does that split exist ? | 12:24 |
| tristan | I don't think so, looking at https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/_variables.pyx#L172 | 12:25 |
| tristan | _check_for_missing and _check_cycles | 12:26 |
| benschubert | oh, they used to exist | 12:26 |
| tristan | Those are called unconditionally anyway | 12:26 |
| tristan | In the current setup, every variable should be resolved only once | 12:27 |
| tristan | And the added explicit check() (only happens on non-junctions) ensures that every variable is resolved | 12:27 |
| tristan | Note: https://gitlab.com/BuildStream/buildstream/-/blob/tristan/partial-variables/src/buildstream/_variables.pyx#L266 | 12:28 |
| tristan | This queues dependencies of a value for resolution | 12:28 |
| tristan | and https://gitlab.com/BuildStream/buildstream/-/blob/tristan/partial-variables/src/buildstream/_variables.pyx#L396 | 12:28 |
| tristan | That decides that a value has no dependencies if it has already been resolved | 12:28 |
| tristan | Maybe we could have a quicker exit | 12:29 |
| tristan | And avoid queuing the dependency itself if it's already resolved | 12:29 |
| tristan | on this line: https://gitlab.com/BuildStream/buildstream/-/blob/tristan/partial-variables/src/buildstream/_variables.pyx#L263 | 12:30 |
| benschubert | https://gitlab.com/BuildStream/buildstream/-/blob/tristan/partial-variables/src/buildstream/_variables.pyx#L171 also seems pretty inefficient :) | 12:30 |
| benschubert | I can probably have a look today/tomorrow if I can understand the hotspot here if you want | 12:30 |
| benschubert | but with the current state, I don't think we should merge it like that | 12:31 |
| tristan | benschubert, I have a local change which removes that and buys some cycles indeed | 12:31 |
| benschubert | let me know if you want me to run the big benchmarks, they take roughly 30-45 minutes to run | 12:32 |
| tristan | benschubert, My local branch changes that for `for varname in self._values.keys(): ... self._expand(varname, None)` | 12:32 |
| tristan | And also removes the unconditional ProvenanceInformation() creation at `_expand()` entry | 12:32 |
| tristan | But I think the difference is tiny :-S | 12:33 |
| tristan | benschubert, and you don't like provenance identity based variable pool reuse ? | 12:34 |
| tristan | I think there are probably a lot of elements in a given project which do not override/define variables | 12:34 |
| benschubert | we can try, I'm not sure I get 100% of that what would entail | 12:34 |
| benschubert | and if we gain a lot by doing that I'm definitely +1 | 12:34 |
| tristan | For every Element which essentially has their variables defined exactly the same way, we would reuse the same Variables instance entirely | 12:35 |
| benschubert | then that seems worth trying | 12:35 |
| tristan | And we could identify that sameness by comparing provenances of values (with ProvenanceInformation identity comparisons) | 12:36 |
| tristan | So we're given a MappingNode at startup, the MappingNode will be a different instance for every element (probably ?) but the provenances of each value should be the same for each variable, if the YAML composition was equal | 12:37 |
| benschubert | that's true | 12:37 |
| tristan | benschubert, I think it would be a big win, but it would falsely make the big benchmark you are running very fast | 12:38 |
| benschubert | and we could have a dict inside Variables that's global to the class and contains a mapping provenance -> Variables class or something? | 12:38 |
| tristan | since it will be the same Variables instance for every Element | 12:38 |
| benschubert | tristan: I'm fine with that, most of the examples I care about are like that | 12:38 |
| benschubert | we should definitely validate for fsdk or something else that is more "organic" that the solution is also good | 12:39 |
| tristan | The mapping is probably a bit tricky to construct efficiently :-S | 12:39 |
| benschubert | true | 12:39 |
| tristan | It might entail sorting the variable keys | 12:39 |
| benschubert | and might actually be worse if more packages define few variables than none... | 12:39 |
| tristan | I think that often enough we have a lot of reuse of identical variables tables | 12:40 |
| tristan | but if they are mostly different, then it's more expensive definitely | 12:40 |
| tristan | Then again, if MappingNode is deterministic in some way, we don't need to sort | 12:41 |
| tristan | We only need a O(n) accumulation of provenance identities to compose a hash key | 12:41 |
| benschubert | MappingNode uses a `dict`, so it would be sorted | 12:41 |
| tristan | Hmmm :-/ | 12:41 |
| tristan | Only happens to be under the current version of python | 12:42 |
| benschubert | I think we would need to try, more than anything else | 12:42 |
| benschubert | it's in the language definition now | 12:42 |
| benschubert | (py37+) | 12:42 |
| tristan | Dont we still support 3.6 ? | 12:42 |
| tristan | We dropped 3.5 I think recently | 12:42 |
| benschubert | yes, and cpython 3.6 has sorted but it's not in the language def | 12:42 |
| benschubert | so another interpreter targeting python3.6 might not implement that hte same way | 12:43 |
| benschubert | but we don't support anything else than CPython anyways | 12:43 |
| tristan | but we anyway require cpython | 12:43 |
| benschubert | then we are fine | 12:43 |
| * tristan reluctantly relinquishes a particle of trust to python | 12:43 | |
| * tristan looks back over his shoulders and hopes it wont come back to haunt him | 12:44 | |
| benschubert | (and to be fair, I'm not sure we'll still need python3.6 support before 2.0 :D) | 12:44 |
| tristan | I'm not sure that we will notice when python 3.12 comes out and yet-again redefines dictionary behavior | 12:44 |
| benschubert | ah no shoot, python3.6 is until 2012-12 | 12:45 |
| benschubert | tristan: before it was undefined | 12:45 |
| benschubert | now it is defined since 3.7 | 12:45 |
| benschubert | so before it was implementation dependent | 12:45 |
| benschubert | now it's part of the language | 12:45 |
| benschubert | and I think the python community is not ready for a breaking change like python2 -> 3 so for breaking "defined" things we should be fine | 12:45 |
| benschubert | async had some breakages but the APIs where provisional for example | 12:46 |
| tristan | I'll give it a shot, I wonder what is a good way to construct such a dictionary key | 12:46 |
| benschubert | for MappingNode? | 12:47 |
| tristan | maybe first just try to use a dictionary of variable name / provenances as a dictionary | 12:47 |
| tristan | No, a dictionary key for a global dictionary of `Variables` instances | 12:47 |
| benschubert | yeah something like that might work | 12:47 |
| tristan | so just use a compare-by-value, maybe I need to use... id(provenance) | 12:48 |
| tristan | (is that a thing ? ... `id()` ? sounds familiar) | 12:48 |
| tristan | Are the MappingNode keys interned too perhaps ? would make sense for them to be | 12:49 |
| tristan | in that case we could use the identities of strings and provenances | 12:49 |
| benschubert | tristan: they are not | 12:49 |
| benschubert | we _can_ try such a change, but will require benchmarking again | 12:50 |
| benschubert | I'm happy to check that if you want | 12:50 |
| benschubert | Or maybe intern all strings in Nodes? | 12:50 |
| benschubert | that seems aweful though | 12:50 |
| tristan | I don't know about values, but certainly MappingNode keys is a good win | 12:56 |
| benschubert | ah? | 12:56 |
| tristan | Values maybe also would save memory | 12:56 |
| benschubert | did you just try? | 12:56 |
| tristan | But MappingNode keys being interned means we speed up every MappingNode.get_*() call if the provided key is interned | 12:56 |
| tristan | Nope, gonna try it now | 12:57 |
| tristan | Where is the code which constructs Node() from yaml ? | 13:01 |
| tristan | the yaml representer thing | 13:02 |
| tristan | hmmm | 13:02 |
| * tristan is trying to find what passes the `dict` to the MappingNode constructor | 13:03 | |
| benschubert | one sec | 13:03 |
| benschubert | https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/_yaml.pyx#L187 | 13:04 |
| benschubert | but that's no guarantee | 13:05 |
| benschubert | so we might need a wrapper around instead | 13:05 |
| tristan | Aha | 13:05 |
| tristan | _handle_doc_MappingStartEvent | 13:05 |
| tristan | benschubert, As far as I can see, the constructor might as well not take any dict | 13:06 |
| tristan | Only an empty dict is ever given, and things are only assigned via __setitem__ ? | 13:06 |
| benschubert | tristan: either this or my previous link | 13:06 |
| benschubert | so we could get rid of my previous link by having an optimized fonction to set things correctly | 13:07 |
| tristan | Oh wait | 13:07 |
| benschubert | it's kind of a bigger change, and will not be as easy | 13:07 |
| tristan | Could we turn that into: (<MappingNode> self.output[-2])[key] = self.output[-1] ? | 13:07 |
| tristan | And always go through __setitem__ ? | 13:07 |
| benschubert | it might be slower | 13:08 |
| tristan | Or that is a cython/python overhead ? | 13:08 |
| benschubert | I am not 100% sure | 13:08 |
| tristan | So I can just call sys.intern() in that place | 13:08 |
| benschubert | but we can either do this or have a special c-only funciton that does the sys.intern + set | 13:08 |
| benschubert | and have that used in __setitem__ too | 13:08 |
| tristan | Or, just: https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/_yaml.pyx#L175 | 13:09 |
| tristan | benschubert, That is keys and scalars and all strings ? | 13:09 |
| benschubert | I'm not 100% sure | 13:09 |
| benschubert | this whole ruamel stuff is hacky | 13:09 |
| tristan | yeah | 13:10 |
| benschubert | but I'd rather have something done in MappingNode directly | 13:11 |
| tristan | ev.value are the strings | 13:11 |
| benschubert | since we have other places in the code that add keys there | 13:11 |
| benschubert | and we probably want to have everything treated the same there | 13:11 |
| tristan | benschubert, We can't really do that with a public MappingNode.value | 13:11 |
| tristan | because _yaml.py goes and touches it | 13:11 |
| benschubert | tristan: it's "public" as in c-public | 13:12 |
| benschubert | so we can have a wrapper in MappingNode, like MappingNode._set(key, value) | 13:12 |
| benschubert | and we change all our code calling MappingNode.value to use this instead | 13:12 |
| tristan | Ok | 13:12 |
| benschubert | if you dont' want to get sidetracked on that and instead work on other optimizations I can do that tonight probably | 13:13 |
| benschubert | (+benchmarks) | 13:13 |
| tristan | benschubert, I think it's together, and I think I have a good patch for it already | 13:18 |
| tristan | sec, lemme push a branch | 13:18 |
| benschubert | awesome :) | 13:18 |
| tristan | grrr AttributeError: 'buildstream.node.MappingNode' object has no attribute '_set' | 13:26 |
| tristan | this _yaml is done differently than _variables.pyx, and has a .pdx which needs to be understood too | 13:26 |
| benschubert | ah you need to update _node.pxd too | 13:28 |
| benschubert | pxd is for exposing a Cython interface | 13:28 |
| benschubert | otherwise cython doesn't expose anything | 13:28 |
| tristan | I did, with the identical signature, I added it right beside _get() | 13:28 |
| benschubert | did you pip install again? | 13:28 |
| tristan | Oh that worked | 13:29 |
| benschubert | :D | 13:29 |
| tristan | benschubert, I've been testing with `tox -e venv /bin/bash` | 13:29 |
| tristan | But this one required a `git clean -xdff` to get rebuilt properly | 13:29 |
| benschubert | uh weird | 13:29 |
| tristan | interned MappingNode strings seems to not effect performance much (in this specific case of the variables test) | 13:30 |
| tristan | Perhaps a small speed gain | 13:30 |
| tristan | I'll push the patch now so you can see it anyway | 13:30 |
| benschubert | {VW} | 13:31 |
| benschubert | woops | 13:31 |
| benschubert | seems good | 13:31 |
| benschubert | do you want me to see the difference on the debian mock project? | 13:31 |
| tristan | benschubert, https://gitlab.com/BuildStream/buildstream/-/merge_requests/1973/diffs?commit_id=b281d937f8cb4f99547e62cf46efe88458f81144 | 13:34 |
| tristan | benschubert, Sure :) | 13:34 |
| tristan | benschubert, However, I was really doing it mostly so that I could rely on interned keys, and from there construct a speedy key for a Variables lookup table for the other, later optimization | 13:35 |
| tristan | But benchmarking them separately could certainly be helpful | 13:35 |
| benschubert | do you want a benchmark of only this commit? Or the branch? | 13:35 |
| tristan | I have a feeling that change might on it's own speed things up a bit, if MappingNode lookups are usually done with strings obtained from MappingNodes in the first place | 13:36 |
| benschubert | Ok I'll try this on its own | 13:36 |
| tristan | It wont speed up Plugin.configure() routines, because those use hard coded strings | 13:36 |
| benschubert | I think we can do even slightly better however, so will modify it a bit :) | 13:36 |
| tristan | Sure :) | 13:36 |
| tristan | benschubert, I don't know what the effect might be of interning the lookup string in _get(), but I suspect it won't improve things (possibly worsen performance) | 13:37 |
| tristan | you really need to be *using* the interned lookup string from calling code in order to get a speedup (so within Variables code, it should definitely help) | 13:38 |
| benschubert | yeah I'd rather not | 13:38 |
| benschubert | we could also have every hard-coded string interned when used in nodes | 13:38 |
| benschubert | would be a manual process but only interned at import time might give some speedup | 13:38 |
| tristan | If an element interns a string to be used later on in Plugin.configure() for a lookup, it will help... *only* if the lookup string is done classwide | 13:39 |
| tristan | Then you intern the string once and you get speedy lookups for every instance of that plugin | 13:40 |
| tristan | benschubert, that said, it might accidentally "just work", e.g.: if cython implementors thought "It's a literal string, as such it should be interned automatically by the language" | 13:42 |
| tristan | The literal string is anyway going to cost memory, so it would make sense to deduplicate them | 13:43 |
| benschubert | yeah let's already see if the initial change helps :) | 13:43 |
| tristan | Asking in #python, the answer is yes: Literal strings are automatically interned | 13:44 |
| benschubert | ah awesome | 13:46 |
| tristan | if the benchmark is not intensive in lookups, might not make much difference (although we have a good chance of memory savings) | 13:48 |
| *** tristan has quit IRC | 13:53 | |
| douglaswinship | jk | 13:58 |
| douglaswinship | (sorry) | 13:58 |
| benschubert | tristan: added https://gitlab.com/BuildStream/buildstream/-/commit/cb34c95009f8a77ea28b7390a5abbbcc428aa171 on top of your commit, am benchmarking just this change. | 14:02 |
| *** tristan has joined #buildstream | 14:03 | |
| *** ChanServ sets mode: +o tristan | 14:03 | |
| benschubert | tristan: there's also other places in the code where we'd need ot change | 14:19 |
| benschubert | if that alone brings some improvements, do you want me to take over and cleanup everywhere for the interning, so you can focus on the rest? | 14:19 |
| tristan | benschubert, Sure, but I might end up overlapping, at least that part I need | 14:29 |
| benschubert | tristan: I don't expect rebasing to be too hard? I will keep the exact interface from the MappingNode you have set | 14:30 |
| tristan | benschubert, I have a `frozenset({id(key): id(value.get_provenance()) for key, value in mapping_node.items())` which successfully reuses Variables instances :) | 14:30 |
| tristan | But it causes some tests to fail so I have to look a bit deeper | 14:30 |
| benschubert | interesting! | 14:30 |
| benschubert | not sure what's the frozenset meant to do here? | 14:31 |
| tristan | dict on it's own is not hashable | 14:31 |
| tristan | so cannot use a dict as a dict key | 14:31 |
| tristan | neither are lists apparently | 14:31 |
| tristan | when running `bst show target.bst` in tests/format/junctions/nested, I get a lot of matches as I would have expected | 14:33 |
| tristan | that so far is at least promising :) | 14:33 |
| * tristan goes to obtain some food and may tinker a bit later in the wee hours of the morning before sleeping :) | 14:34 | |
| benschubert | ah right, I'd actually be surprised if you get some speedup with having to hash all of this :) let's hope | 14:36 |
| benschubert | wouldn't the provenance of 'MappingNode' itself be enough? | 14:36 |
| tomaz | hey, I see that we have a bit of snap code within buildstream, is this still open or someone forgot to close? https://gitlab.com/BuildStream/buildstream/-/issues/650 | 14:39 |
| coldtom | i'd argue that it's still an issue, as the snap_image plugin lives in freedesktop-sdk only, valentind has more context than me though | 14:43 |
| benschubert | tristan: it's slightly slower with the change on the dummy debian project | 14:54 |
| tristan | benschubert, the provenance of the MappingNode itself doesnt guarantee me that none of the child values have been composited | 15:05 |
| tristan | which I guess explains the failing tests | 15:06 |
| benschubert | ah good point | 15:06 |
| tristan | Because the same is true for nested dicts | 15:06 |
| tristan | I could have the same provenance identity for a child key which is a dict, but that child's child being composited invalidates the mapping node | 15:06 |
| tristan | Surely a recursive collection of provenance identities is still way cheaper than a resolution of Variables, though | 15:07 |
| tristan | benschubert, how does memory consumption compare btw ? | 15:14 |
| benschubert | no change | 15:14 |
| tristan | Hmmm | 15:14 |
| tristan | Makes me wonder if the strings are already interned by ruamel internally | 15:14 |
| tristan | and we're just adding overhead of re-looking them up with sys.intern() | 15:14 |
| benschubert | possible, could check the ruamel codebase | 15:14 |
| tristan | then again, the initially loaded yaml may not be all that much in this project, and just reused/recomposited many times | 15:15 |
| tristan | maybe the keys are not as redundant in memory as one might expect | 15:15 |
| benschubert | yeah | 15:16 |
| * benschubert really doesn't like browsing code on sourceforge | 15:17 | |
| tristan | composition will not end up duplicating them, the strings are immutable anyway | 15:17 |
| tristan | nah | 15:17 |
| tristan | I always browse code in .tox/ | 15:17 |
| benschubert | haha | 15:17 |
| tristan | I don't have grep in sourceforge anyway :) | 15:17 |
| tristan | what use is that | 15:18 |
| benschubert | I like gitlab search/github search as it allows also to look if issues are mentioning this :) | 15:19 |
| *** santi has quit IRC | 16:04 | |
| *** santi has joined #buildstream | 16:05 | |
| tomaz | do you guys know about buildstream.co.uk ? | 16:14 |
| tristan | benschubert, around still ? | 17:46 |
| benschubert | yep! | 17:46 |
| benschubert | tristan: ^ | 17:47 |
| tristan | I've been tinkering, found that there is of course no need for recursion to compose this key | 17:47 |
| tristan | Because variables are a flat dict | 17:47 |
| tristan | But we cannot use the id() of a Provenance | 17:47 |
| tristan | because ProvenanceInformation() has been refactored into something that is constructed on demand ! | 17:47 |
| benschubert | oh right | 17:47 |
| tristan | Here I was thinking it was something which could uniquely identify the node | 17:48 |
| tristan | Maybe I can use the ID of the value Nodes ? | 17:48 |
| tristan | lemme check if they remain constant | 17:48 |
| tristan | The caching is otherwise working fine if I use value.as_str() | 17:48 |
| tristan | but that's of course undesirably expensive | 17:48 |
| benschubert | yeah their ID might be fine | 17:51 |
| tristan | So long as composition doesnt clone them, which I suppose it shouldn't anyway | 17:51 |
| *** santi has quit IRC | 17:52 | |
| tristan | Nope :-S | 17:52 |
| tristan | ScalarNodes like 'prefix' values from ~/buildstream/src/buildstream/data/projectconfig.yaml have different IDs from one element to another, in the same project/session | 17:53 |
| tristan | Oh damn | 17:54 |
| tristan | Maybe this is futile after all | 17:54 |
| benschubert | ah I think the reason for that is that plugins can access their nodes, so we clone them per plugin | 17:55 |
| tristan | Ah of course | 17:56 |
| tristan | But, there is another disastrous thing | 17:56 |
| tristan | https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/element.py#L286 | 17:56 |
| tristan | That line kind of throws the whole thing off :-S | 17:56 |
| benschubert | hahaha, oh yeah of course -_-' | 17:57 |
| tristan | That would mean that, any caching/reuse might instead require Variables.clone() with a partial re-resolution of some vars referencing 'element-name' :-S | 17:57 |
| benschubert | yeah no, let's forget that | 17:57 |
| benschubert | too many places where it could go wrong | 17:57 |
| tristan | Yeah, do we really ever use 'element-name' at all for anything though ? | 17:57 |
| benschubert | for some plugins it can be nice | 17:58 |
| benschubert | ah but that would end up with ".bst" anyways right? | 17:58 |
| benschubert | so that's not idea | 17:58 |
| benschubert | or maybe we should keep it but remove the .bst? | 17:58 |
| tristan | Seems like we really should be able to avoid a lot, and otherwise, people can always name stuff in the yaml | 17:58 |
| tristan | I don't know what use case you're thinking of where .bst is undesirable | 17:59 |
| tristan | I don't really understand the usecase of why it's desirable either, I mean it does 'seem neat' | 17:59 |
| tristan | but otherwise | 17:59 |
| benschubert | if at a project level you force the 'package' name or 'library' name to match the name of the element, your plugin could infer much more and sometimes require very little setup | 17:59 |
| benschubert | I for sure know a few places where there is such mapping | 18:00 |
| tristan | I suppose if we had composite plugins then the composite plugin implementor would still be able to automatically assign names to the elements which are declared inside the same .bst file | 18:00 |
| *** tomaz has quit IRC | 18:01 | |
| tristan | myeah so, I guess we should ditch the idea of caching entire Variables instances, although, it was attractive | 18:01 |
| tristan | Hmmm | 18:01 |
| tristan | It could be an exception | 18:01 |
| tristan | well, a bit complex | 18:02 |
| tristan | ugh | 18:02 |
| benschubert | tristan: you might be interested: https://gitlab.com/BuildStream/buildstream/-/compare/master...bschubert%2Fno-multiprocessing is what I've got currently, I've got still problems with lingering threads and must then make sure that everything works really correctly wrt terminate and pause, but otherwise it's looking that we will be able to simplify the codebase a lot | 18:04 |
| tristan | That's gonna be a big review, I don't think I can context switch very well at this time :-S | 18:05 |
| tristan | Lets give ourselves another day or two, and if I can get acceptable performance here, I'll put my full attention on that review ? | 18:06 |
| benschubert | ah don't right now | 18:06 |
| benschubert | I just wanted to give a head's up that I'm working on it and think it will be a good thing :) | 18:06 |
| benschubert | I'll still need 1-2 weeks before I can put it to review I think | 18:06 |
| tristan | Yeah I figured you'd still have some road to go :) | 18:06 |
| benschubert | It's really more convoluted than I had oped | 18:06 |
| tristan | haha ok, overly optimistic here :) | 18:07 |
| benschubert | but oh well | 18:07 |
| tristan | I'm curious why the current variables code is consistently more performant with `bst show --format '%{deps}'`, and what that tradeoff means | 18:08 |
| tristan | it might be that the overheads are not where we think they are | 18:09 |
| tristan | i.e. in the tight resolution loops | 18:09 |
| benschubert | that's a possibility | 18:09 |
| benschubert | and profiling the cython code is tricky | 18:09 |
| benschubert | unless you like perf/strace | 18:10 |
| tristan | strace doesnt sound fun | 18:10 |
| tristan | cachegrind is nice | 18:10 |
| tristan | but maybe not possible | 18:10 |
| tristan | might be able to hone in a bit with hand written micro-benchmarking | 18:11 |
| tristan | maybe there is just a constant overhead of allocating the Value() objects | 18:11 |
| tristan | and not a (severe) algorithmic problem | 18:12 |
| benschubert | that's also a possiblity indeed | 18:13 |
| tristan | Maybe inline C is better ? | 18:13 |
| tristan | inline C for struct allocation and then cython functions to operate on them ? | 18:13 |
| benschubert | it's probably worth a shot | 18:14 |
| tristan | I'll have to figure out how all of that works | 18:14 |
| tristan | do you have a good link which describes how to use cython this way ? I've basically just been googling as-needed and getting results of varying quality | 18:15 |
| benschubert | tristan: one thing you can try before, is add `annotate=True` in https://gitlab.com/BuildStream/buildstream/-/blob/master/setup.py#L271, then `pip install` again, it should generate a .html for each pyx file | 18:16 |
| benschubert | that will allow you to see the equivalent code genrated | 18:16 |
| benschubert | more yellow means more python interaction | 18:16 |
| benschubert | less yellow is better, white means pure C | 18:16 |
| tristan | oh wait, I think I've got a hint | 18:17 |
| tristan | benschubert, there *might* actually be a difference between __init__ and __cinit__ | 18:19 |
| benschubert | ah there is! | 18:19 |
| benschubert | sorry did I miss that? | 18:19 |
| * tristan says with blatent naivety | 18:19 | |
| benschubert | basically, only use __cinit__ if you can | 18:20 |
| tristan | well, that appears to make a small difference | 18:20 |
| * tristan thinks he has all of his def/cdef/cpdef correct (with maybe one minor fix here) | 18:23 | |
| benschubert | you can use annotate=True to see that quickly :) | 18:24 |
| tristan | I ended up doing that manually the other day (cython -a _variables.pyx) | 18:25 |
| tristan | lemme try again | 18:25 |
| tristan | or use the setup trick | 18:25 |
| benschubert | ah yeah -a also works | 18:26 |
| tristan | So the yellow lines are the good ones or the bad ones ? | 18:27 |
| tristan | The bad ones I guess ? | 18:27 |
| benschubert | bad | 18:27 |
| benschubert | the more yellow the worse it is :) | 18:27 |
| benschubert | white is perfect C | 18:27 |
| benschubert | "perfect" | 18:27 |
| tristan | Ok we can improve expand() | 18:28 |
| tristan | it calls into itself, recursing through a dictionary where stuff gets expanded | 18:28 |
| tristan | It needs to be called in python, but it could call into itself in C | 18:28 |
| benschubert | yep that would be good | 18:28 |
| benschubert | either cpdef or def then a wrapper in C (cdef _expand) | 18:29 |
| benschubert | I usually prefer the second one since it's more explicit | 18:29 |
| tristan | well wait, if it's already cpdef, then when it calls into itself it should be in C ? | 18:30 |
| tristan | Ok I'll change it anyway | 18:30 |
| tristan | cause I totally agree, that's less sloppy | 18:30 |
| benschubert | I am not 100% sure | 18:31 |
| tristan | Do you know what is the difference from bright and half yellow ? | 18:33 |
| benschubert | the brighter the more it uses python APIs | 18:34 |
| benschubert | you might want to split if you are doing multiple things per line to know exactly what's happening | 18:34 |
| tristan | Hmmm | 18:34 |
| tristan | benschubert, https://gitlab.com/BuildStream/buildstream/-/blob/tristan/partial-variables/src/buildstream/_variables.pyx#L157 | 18:34 |
| tristan | I don't understand why that line would be bright yellow | 18:34 |
| tristan | it looks like cython frowns upon for loops | 18:35 |
| benschubert | https://gitlab.com/BuildStream/buildstream/-/blob/tristan/partial-variables/src/buildstream/_variables.pyx#L396 you are returning an object here | 18:36 |
| benschubert | not a list | 18:36 |
| benschubert | cdef list dependencies() | 18:36 |
| tristan | oh so I need to declare it as a list | 18:36 |
| benschubert | should help | 18:36 |
| benschubert | yep | 18:36 |
| benschubert | otherwise it returns `PyObject*` | 18:36 |
| tristan | still bright yellow | 18:38 |
| benschubert | also: if you click on a yellow line it will show you the code | 18:38 |
| tristan | ooooooh | 18:38 |
| benschubert | Yeah :D | 18:39 |
| tristan | And the red stuff in the code is "bad" I guess ? | 18:41 |
| tristan | PyMethod_Check | 18:42 |
| tristan | __Pyx_PyObject_CallOneArg | 18:42 |
| benschubert | Yeah | 18:43 |
| tristan | iter_referee, iter_provenance, iter_name_set = pending.pop() looks expensive | 18:43 |
| tristan | unpacking tuples has a lot of checks | 18:43 |
| tristan | for some reason strings have a lot of uncertainty about them too | 18:43 |
| tristan | checking for unicode or which one of the dozens of kinds of strings are in use | 18:44 |
| benschubert | :/ | 18:44 |
| benschubert | I wonder if we should go deeper in C | 18:45 |
| benschubert | it's less user friendly, harder to follow | 18:45 |
| benschubert | but it might be good if used in very targeted places | 18:46 |
| tristan | I wonder if that is better to create a cython class and instantiate it instead of pushing tuples | 18:46 |
| benschubert | oh yeah! | 18:47 |
| tristan | it might mean more consistent memory slices (less thrashing), and less typechecking on unpack | 18:47 |
| benschubert | that's actually very probable | 18:47 |
| tristan | ok well, it's very late now, I think I'll give it a rest and revisit tomorrow | 18:48 |
| tristan | but now I've got a lot to work with ! | 18:48 |
| tristan | reduce yellowness ! | 18:48 |
| benschubert | tristan: sure :) I'm looking forward to seeing improvements, and am happy more people are taking on the cython part :) | 18:50 |
| *** phildawson has quit IRC | 22:28 | |
| *** benschubert has quit IRC | 23:22 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!