drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hanifi Gunes <hgu...@maprtech.com>
Subject Re: Request - hold off on merging to master for 48 hours
Date Fri, 31 Jul 2015 16:15:03 GMT
+1 to Jacques, it'd be nice if we had the core changes easily re/viewable.
Also, would it make sense at this point to split the change set into
smaller patches as there seems more work to do now?


H+

On Fri, Jul 31, 2015 at 7:06 PM, Jacques Nadeau <jacques@dremio.com> wrote:

> 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
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message