Short version: I'll call it quits on the merge moratorium for now. Thank you to everyone for participating. Merge away.
In the precommit suite, one query fails with an illegal reference counting exception from the external sort, and Steven has found that for me. This is the closest I've ever gotten. On future attempts to commit after rebasing, I'm going to be counting on other file owners a lot more to get through that quickly, rather than trying to find all the newly introduced problems myself. Long version: when I run the performance suite, the results with the non-locking version of the allocator are terrible. Worse than the locking implementation of the allocator (I still have both on separate branches). When we ran this on the locking implementation, there was roughly a 20% performance degradation, and consensus was that this was too much to accept the change. The locking implementation uses a single lock for all allocators. (Yes, I know that sounds heavy-handed, but it wasn't the first choice. There was a prior implementation that used a lock per allocator, but that one got deadlocks all the time because it couldn't ensure consistent lock acquisition orders when allocators went to their parents to get more space, combined with allocators locking each other to transfer or share buffer ownership.) I thought I'd solve this with a non-locking implementation. In this version, the variables that are used to track the state of an allocator re its available space, and how it is used, are kept in a small inner class; the allocator has an AtomicReference to that. A space allocation consists of getting that reference, making a clone of it, and then making all the necessary changes to the clone. To commit the space transaction, I try to swap it in with AtomicReference.compareAndSet(). If that fails, the transaction is retried. I expected that there would be no failures with leaf allocators, because they're only used by the thread doing the fragment's work. Only the root should have seen contention. But the performance cluster test showed the performance for this implementation to be five times worse than the current master (yes 5x, not just 20% worse like the locking implementation). I've done some quick sanity checks today, but don't see anything obviously silly. I will investigate a little further -- I've already come up with a couple of potential issues, but I need to do a couple experiments with it over the next few hours (and which wouldn't leave enough time to do the merge by the 48 hour deadline). If I can't over come those issues, then I will at least go for obtaining the root allocator from a factory, and set things up so that the current and new allocator can co-exist, because the new one definitely catches a lot more problems -- we should be running tests with it on. Hopefully I can overcome the issues, shortly, because I think the accounting is much better (that's why it catches more problems), and we need that in order to find our ongoing slow memory leak. On Wed, Jul 29, 2015 at 4:00 PM, Jacques Nadeau <[email protected]> wrote: > Makes sense. > > -- > Jacques Nadeau > CTO and Co-Founder, Dremio > > On Wed, Jul 29, 2015 at 3:32 PM, Chris Westin <[email protected]> > wrote: > > > Ordinarily, I would agree. However, in this particular case, some other > > folks wanted me closer to master so they could use my branch to track > down > > problems in new code. Also, the problems I was seeing were in code I'm > not > > familiar with, but there had been several recent commits claiming to fix > > memory issues there. So I wanted to see if the problems I was seeing had > > been taken care of. Sure enough, my initial testing shows that the > problems > > I was trying to fix had already been fixed by others -- they went away > > after I rebased. In this case, chasing master saved me from having to > track > > all of those down myself, and duplicating the work. I'm hoping that there > > weren't any significant new ones introduced. Testing is proceeding. > > > > On Wed, Jul 29, 2015 at 1:59 PM, Parth Chandra <[email protected]> > wrote: > > > > > I think the idea (some of the idea is mine I'm afraid) is to allow > Chris > > to > > > catch up and rebase, not to have it reviewed and merged in two days. > > > At the moment the problem is that every time he rebases, some test > breaks > > > and while he's chasing that down, the master moves ahead. > > > If we get this 2 day break, we can get close enough to master and share > > the > > > changes (a pre-review perhaps). > > > Also, agree that perhaps the patch could be split into smaller pieces. > > Not > > > renaming the allocator would in fact save several files from being > > included > > > in the patch. > > > > > > > > > P > > > > > > > > > On Wed, Jul 29, 2015 at 12:17 PM, Jacques Nadeau <[email protected]> > > > wrote: > > > > > > > In general, this type of request makes a lot of sense. That said, we > > > > should get to +1 before we hold master. > > > > > > > > The last changes that were posted on DRILL-1942 are more than a month > > > old. > > > > The patch touched ~100 files and was thousands of lines. It had an > > issue > > > > that we identified. Since then, you exposed very little to the > > community > > > > on your progress. It seems unrealistic to provide a large update to > > this > > > > patch and expect review and merge within 48 hours. Had you been > > > exposing & > > > > discussing your work over the last month and the community agreed > that > > it > > > > was ready for merge, your ask here would seem more feasible. > > > > > > > > My suggestion is to stop chasing master AND break your patch into > > smaller > > > > pieces. Looking at the old patch, the vast majority of changes have > > very > > > > little to do with a new allocator functionality. For example, > renaming > > > the > > > > allocator could be merged without changing the underlying > > implementation > > > > (that would substantially reduce the patch size). > > > > > > > > For the allocator specifically, let's get the code right first. Then > > we > > > > can work as a community to minimize the effort to get it merged. > > > > > > > > > > > > > > > > -- > > > > Jacques Nadeau > > > > CTO and Co-Founder, Dremio > > > > > > > > On Wed, Jul 29, 2015 at 11:41 AM, Chris Westin < > > [email protected]> > > > > wrote: > > > > > > > > > I've got a large patch that includes a completely rewritten direct > > > memory > > > > > allocator (replaces TopLevelAllocator). > > > > > > > > > > The space accounting is much tighter than with the current > > > > implementation, > > > > > and it catches a lot more problems than the current implementation > > > does. > > > > It > > > > > also fixes issues with accounting around the use of shared buffers, > > and > > > > > buffer ownership transfer (used by the RPC layer to hand off > buffers > > to > > > > > fragments that will do work). > > > > > > > > > > It's been an ongoing battle to get this in, because every time I > get > > > > close, > > > > > I rebase, and it finds more new problems (apparently introduced by > > > other > > > > > work done since my last rebase). These take time to track down and > > fix, > > > > > because they're often in areas of the code I don't know. > > > > > > > > > > It looks like I'm very close right now. I rebased against > > apache/master > > > > on > > > > > Friday. All the unit tests passed. All of our internal tests passed > > > > except > > > > > for one query, which takes an IllegalReferenceCountException (it > > looks > > > > like > > > > > a DrillBuf is being released one more time than it should be). > > > > > > > > > > So, in order to keep the gap from getting wide again (it looks like > > I'm > > > > > already a couple of commits behind, but hopefully they don't > > introduce > > > > more > > > > > issues), I'm asking that folks hold off on merging into master for > 48 > > > > hours > > > > > from now -- that's until about noon on Friday PST. I'm hoping that > > will > > > > > give me the time needed to finally get this in. If things go wrong > > with > > > > my > > > > > current patching, or I discover other problems, or can't find the > > > illegal > > > > > reference count issue by then, I'll post a message and open things > up > > > > > again. Meanwhile, you can still pull, do work, make pull requests, > > and > > > > get > > > > > them reviewed; just don't merge them to master. > > > > > > > > > > Can we agree to this? > > > > > > > > > > Chris > > > > > > > > > > > > > > >
