You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Chris Westin <ch...@gmail.com> on 2015/07/29 20:41:04 UTC
Request - hold off on merging to master for 48 hours
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
Re: Request - hold off on merging to master for 48 hours
Posted by Jacques Nadeau <ja...@dremio.com>.
My thought is more general...why chase master? Let's get the algorithm and
infrastructure right on some branch. Then I'm all for pausing for rebase
purposes. There isn't enough fundamental stuff changing that this should me
a giant issue. There is no reason we can't be 99% approved based off the
1.0 release branch. Then worry about the difference. If some things, like
renaming and code organization, are cosmetic, get those merged asap.
On Jul 29, 2015 1:59 PM, "Parth Chandra" <pa...@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 <ja...@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 <ch...@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
> > >
> >
>
Re: Request - hold off on merging to master for 48 hours
Posted by Hanifi Gunes <hg...@maprtech.com>.
+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 <ja...@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 <ch...@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 <ja...@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 <pa...@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
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Request - hold off on merging to master for 48 hours
Posted by Jacques Nadeau <ja...@dremio.com>.
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 <ch...@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 <ja...@dremio.com>
> wrote:
>
> > Makes sense.
> >
> > --
> > Jacques Nadeau
> > CTO and Co-Founder, Dremio
> >
> > On Wed, Jul 29, 2015 at 3:32 PM, Chris Westin <ch...@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 <pa...@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
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Request - hold off on merging to master for 48 hours
Posted by Chris Westin <ch...@gmail.com>.
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 <ja...@dremio.com> wrote:
> Makes sense.
>
> --
> Jacques Nadeau
> CTO and Co-Founder, Dremio
>
> On Wed, Jul 29, 2015 at 3:32 PM, Chris Westin <ch...@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 <pa...@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 <ja...@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
> > > > >
> > > >
> > >
> >
>
Re: Request - hold off on merging to master for 48 hours
Posted by Jacques Nadeau <ja...@dremio.com>.
Makes sense.
--
Jacques Nadeau
CTO and Co-Founder, Dremio
On Wed, Jul 29, 2015 at 3:32 PM, Chris Westin <ch...@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 <pa...@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 <ja...@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
> > > >
> > >
> >
>
Re: Request - hold off on merging to master for 48 hours
Posted by Chris Westin <ch...@gmail.com>.
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 <pa...@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 <ja...@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 <ch...@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
> > >
> >
>
Re: Request - hold off on merging to master for 48 hours
Posted by Parth Chandra <pa...@apache.org>.
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 <ja...@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 <ch...@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
> >
>
Re: Request - hold off on merging to master for 48 hours
Posted by Jacques Nadeau <ja...@dremio.com>.
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 <ch...@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
>
Re: Request - hold off on merging to master for 48 hours
Posted by Hanifi Gunes <hg...@maprtech.com>.
I am fine with holding off my check-ins until Friday noon.
On Wed, Jul 29, 2015 at 9:41 PM, Chris Westin <ch...@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
>