Mailing-List: contact dev-help@drill.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@drill.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CAMGsNzDS-jnSppa2z2QhgefPO7MyOQWNHz-G3xA1czM8d43PeQ@mail.gmail.com>
References: 
 <CAMGsNzCQqM3=8k9cBcm_RM_KKDYWV3ayDiRhXoeA0YOxwMor8Q@mail.gmail.com>
	<CAJrw0OShS5owHLdmNMNbsfFtP8Nx3zGXjtmX5vVhezvDm-bsSA@mail.gmail.com>
	<CAAOiHjE4Pf34RNgamzXAsOo5WYis+-veJotY1hF6G2rdGNhsnQ@mail.gmail.com>
	<CAMGsNzBMw1vxypsJoxK25c+Lg5GZ61P757rNE7L5qKCZFrmYdw@mail.gmail.com>
	<CAJrw0OS7h+T_zo_4N7WEgSEhjd5b73S_+25SbePE_VN5xKTzEQ@mail.gmail.com>
	<CAMGsNzDS-jnSppa2z2QhgefPO7MyOQWNHz-G3xA1czM8d43PeQ@mail.gmail.com>
Date: Fri, 31 Jul 2015 09:06:50 -0700
Message-ID: 
 <CAJrw0ORXLzeKJPTQZU=jhG-sBojd604-3xBbqLzkaA-N--yB0w@mail.gmail.com>
Subject: Re: Request - hold off on merging to master for 48 hours
From: Jacques Nadeau <jacques@dremio.com>
To: dev@drill.apache.org
Content-Type: multipart/alternative; boundary=047d7b5da8b5fc9c9d051c2e019d

--047d7b5da8b5fc9c9d051c2e019d
Content-Type: text/plain; charset=UTF-8

That sounds frustrating.

I agree that we need to get this merged.  The old allocator is sloppy about
accounting at best.  Lets work together on trying to come up with a
solution. Can you point us at the current branch so other people can
provide some brainstorming?

--
Jacques Nadeau
CTO and Co-Founder, Dremio

On Thu, Jul 30, 2015 at 4:00 PM, Chris Westin <chriswestin42@gmail.com>
wrote:

> 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 <jacques@dremio.com>
> wrote:
>
> > Makes sense.
> >
> > --
> > Jacques Nadeau
> > CTO and Co-Founder, Dremio
> >
> > On Wed, Jul 29, 2015 at 3:32 PM, Chris Westin <chriswestin42@gmail.com>
> > 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 <parthc@apache.org>
> > 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 <jacques@dremio.com
> >
> > > > 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 <
> > > chriswestin42@gmail.com>
> > > > > 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
> > > > > >
> > > > >
> > > >
> > >
> >
>

--047d7b5da8b5fc9c9d051c2e019d--