On Wed, Feb 28, 2024 at 3:44 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote: > > On Tue, Feb 27, 2024 at 11:24 PM Ian Rogers <irogers@xxxxxxxxxx> wrote: > > > > On Tue, Feb 27, 2024 at 10:40 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote: > > > > > > On Tue, Feb 27, 2024 at 1:42 PM Ian Rogers <irogers@xxxxxxxxxx> wrote: > > > > > > > > On Tue, Feb 27, 2024 at 11:17 AM Arnaldo Carvalho de Melo > > > > <acme@xxxxxxxxxx> wrote: > > > > > > > > > > On Tue, Feb 27, 2024 at 09:31:33AM -0800, Namhyung Kim wrote: > > > > > > I can see some other differences like machine__findnew_thread() > > > > > > which I think is due to the locking change. Maybe we can fix the > > > > > > problem before moving the code and let the code move simple. > > > > > > > > > > I was going to suggest that, agreed. > > > > > > > > > > We may start doing a refactoring, then find a bug, at that point we > > > > > first fix the problem them go back to refactoring. > > > > > > > > Sure I do this all the time. Your typical complaint on the v+1 patch > > > > set is to move the bug fixes to the front of the changes. On the v+2 > > > > patch set the bug fixes get applied but not the rest of the patch > > > > series, etc. > > > > > > > > Here we are refactoring code for an rb-tree implementation of threads > > > > and worrying about its correctness. There's no indication it's not > > > > correct, it is largely copy and paste, there is also good evidence in > > > > the locking disciple it is more correct. The next patch deletes that > > > > implementation, replacing it with a hash table. Were I not trying to > > > > break things apart I could squash those 2 patches together, but I've > > > > tried to do the right thing. Now we're trying to micro correct, break > > > > apart, etc. a state that gets deleted. A reviewer could equally > > > > criticise this being 2 changes rather than 1, and the cognitive load > > > > of having to look at code that gets deleted. At some point it is a > > > > judgement call, and I think this patch is actually the right size. I > > > > think what is missing here is some motivation in the commit message to > > > > the findnew refactoring and so I'll add that. > > > > > > I'm not against your approach and actually appreciate your effort > > > to split rb-tree refactoring and hash table introduction. What I'm > > > asking is just to separate out the code moving. I think you can do > > > whatever you want in the current file. Once you have the final code > > > you can move it to its own file exactly the same. When I look at this > > > commit, say a few years later, I won't expect a commit that says > > > moving something to a new file has other changes. > > > > The problem is that the code in machine treats the threads lock as if > > it is a lock in machine. So there is __machine__findnew_thread which > > implies the thread lock is held. This change is making threads its own > > separate concept/collection and the lock belongs with that collection. > > Most of the implementation of threads__findnew matches > > __machine__findnew_thread, so we may be able to engineer a smaller > > line diff by moving "__machine__findnew_thread" code into threads.c, > > then renaming it to build the collection, etc. We could also build the > > threads collection inside of machine and then in a separate change > > move it to threads.[ch]. In the commit history this seems muddier > > than just splitting out threads as a collection. Also, some of the API > > design choices are motivated more by the hash table implementation of > > the next patch than trying to have a good rbtree abstracted collection > > of threads. Essentially it'd be engineering a collection of threads > > but only with a view to delete it in the next patch. I don't think it > > would be for the best and the commit history for deleted code is > > unlikely to be looked upon. > > I think the conversation is repeating. :) Why not do this? > > 1. refactor threads code in machine.c and fix the locking > 2. move threads code to its own file > 3. use hash table in threads Step 3 is patch 5 in this series. I can split this patch into steps 1 and 2 as you say in v3. I'm not sure why step 2 is going to be of any use as the code will match that of step 1, code that the next patch then deletes. Anyway, it's a plan for v3. Thanks, Ian > Thanks, > Namhyung
