On Mon, Apr 23, 2018 at 7:09 PM, Elijah Newren <newren@xxxxxxxxx> wrote: > Hi, > > On Sun, Apr 22, 2018 at 5:38 AM, Duy Nguyen <pclouds@xxxxxxxxx> wrote: >>> - there's a better, more performant fix or there is some way to actually >>> share a split_index between two independent index_state objects. >> >> A cleaner way of doing this would be something to the line [1] >> >> move_index_extensions(&o->result, o->dst_index); >> >> near the end of this function. This could be where we compare the >> result index with the source index's shared file and see if it's worth >> keeping the shared index or not. Shared index is designed to work with >> huge index files though, any operations that go through all index >> entries will usually not be cheap. But at least it's safer. > > Yeah, it looks like move_index_extensions() currently has no logic for > the split_index. Adding it sounds to me like a patch series of its > own, and I'm keen to limit additional changes since my patch series > already broke things pretty badly once already. Oh I'm not suggesting that you do it. I was simply pointing out something I saw while I looked at this patch and surrounding area. And it's definitely should be done separately (by whoever) since merge logic is quite twisted as I understand it (then top it off with rename logic) >> [1] To me the second parameter should be src_index, not dst_index. >> We're copying entries from _source_ index to "result" and we should >> also copy extensions from the source index. That line happens to work >> only when dst_index is the same as src_index, which is the common use >> case so far. > > That makes sense; this sounds like another fix that should be > submitted. Did you want to submit a patch making that change? Do you > want me to? I did not look careful enough to make sure it was right and submit a patch. But it sounds like it could be another regression if dst_index is now not the same as src_index (sorry I didn't look at your whole stories and don't if dst_index != src_index is a new thing or not). If dst_index is new, moving extensions from that to result index is basically no-op, in other words we fail to copy necessary extensions over. -- Duy
