Re: [DISCUSS] DrillBuf

[Paul Rogers]

Hi Vlad,

Thanks for the clarifications. My general comment is that it is always good to 
refactor things if that is the fastest way to achieve some goal. It is not 
clear, however, what the goal is here other than code improvement. Since Drill 
still has plenty of opportunities for improvement that will help users, I guess 
it's a bit unclear why we'd clean up code just for its own sake if doing so 
will entail a large amount of work and provide no user benefit.

I say this because I've done quite a bit of work in surrounding code and 
learned how much work it takes to make these kinds of changes and then 
stabilize the result.

On the other hand, if you are working on a memory-related project that is 
making major changes, and these issues are getting in your way, then 
refactoring could well be the fastest way to achieve your project goals. Are 
you working on such a project?

> why was it necessary to introduce DrillBuf and/or UnsafeDirectLittleEndian? 
> What functionality do those classes provide that existing Netty classes do 
> not?
> IMO, it will be good to make DrillBuf code simpler and consistent.

Good questions!  So, it seems that DrillBuf turns out to be a bit of a muddle. 
But, unless it is preventing us from making some desired change, or changing 
things will provide a significant performance boost, I'd guess I'd just hold my 
nose and leave it as is for now.

The same argument applies, by the way, to the value vectors. The value vector 
classes become almost entirely redundant once the row set mechanisms are 
adopted. But, I suspect that value vectors will live on anyway until there is a 
reason to do something with them.

> Question is whether or not to check that reference count is not zero every 
> time DrillBuf is used (see ensureAccessible()).

IMHO, there is no reason to check on every access, except in a "paranoid" debug 
mode. Removing the check might provide a nice little performance bump. Avoiding 
bounds and ref checks was one of the goals of the "unchecked" methods that I 
had in DrillBuf but which we decided to remove... If it turns out that the 
methods used by the row set abstractions do, in fact, do bounds checks, then 
this is a strong case to put the "unsafe" (unchecked) methods back.

> 5. Moving DrillBuf to a different package


I agree with your explanation. However, I assume the original authors were 
forced to put DrillBuf in the Netty package for some reason or other. If that 
reason is no longer valid, then it is usually pretty simple to have your IDE 
move the class to a different package and adjust all its references. This 
improvement, if possible, seems low cost and so might be worth doing.

On the other hand, if the move causes things to break, which causes effort to 
go into changing things, I guess I'd wonder why we can't just leave well enough 
alone and focus on things which are actually broken or could use a performance 
boost.

In short, I'm all for refactoring when it helps us deliver fixes or new 
features to customers. But, I'm struggling to see the user benefit in this 
case. Can you help me to understand the user benefit of these changes?

Thanks,

- Paul

 

On Saturday, April 7, 2018, 9:35:06 PM PDT, Vlad Rozov  
wrote:  
 
 Hi Paul,

My comments in-line.

Thank you,

Vlad

On 4/5/18 20:50, Paul Rogers wrote:
> Hi Vlad,
>>  I'd suggest to keep focus on DrillBuf design and implementation as the only 
>>gate for accessing raw (direct) memory.
> I was doing that. By explaining where DrillBuf fits in the overall design, we 
> see that DrillBuf should be the only access point for direct memory. The 
> context explains why this is the right decision. Changes to DrillBuf should 
> support our design as DrillBuf only exists for that purpose.
My concern is not why Drill adopted Netty as Netty does provide a good 
amount of functionality on top of Java networking and NIO. I also do not 
propose to replace Netty with something else. My primary focus for this 
thread is the design and implementation of the DrillBuf Java class 
itself. Namely, why was it necessary to introduce DrillBuf and/or 
UnsafeDirectLittleEndian? What functionality do those classes provide 
that existing Netty classes do not? Netty already provides memory 
pooling, reference counting, slicing, composite buffers, working with 
direct and heap memory. By looking at the DrillBuf.java git history, the 
DrillBuf was introduced in 2014 and prior to that Netty classes were 
used directly. Unfortunately, the commit that introduced DrillBuf does 
not provide any info why it was introduced and does not have a reference 
to a JIRA.

One may argue that DrillBuf is a way for Drill to encapsulate Netty 
ByteBuf and guard other modules that use DrillBuf from Netty ByteBuf 
API, so if Netty decides to change ByteBuf API in the next major 
release, amount of changes will be limited to DrillBuf only. Problem is 
that DrillBuf inherits from Netty AbstractByteBuf, so the above goal 

Re: [DISCUSS] DrillBuf

[Vlad Rozov]

Hi Paul,

My comments in-line.

Thank you,

Vlad

On 4/5/18 20:50, Paul Rogers wrote:

Hi Vlad,

  I'd suggest to keep focus on DrillBuf design and implementation as the only 
gate for accessing raw (direct) memory.

I was doing that. By explaining where DrillBuf fits in the overall design, we 
see that DrillBuf should be the only access point for direct memory. The 
context explains why this is the right decision. Changes to DrillBuf should 
support our design as DrillBuf only exists for that purpose.
My concern is not why Drill adopted Netty as Netty does provide a good 
amount of functionality on top of Java networking and NIO. I also do not 
propose to replace Netty with something else. My primary focus for this 
thread is the design and implementation of the DrillBuf Java class 
itself. Namely, why was it necessary to introduce DrillBuf and/or 
UnsafeDirectLittleEndian? What functionality do those classes provide 
that existing Netty classes do not? Netty already provides memory 
pooling, reference counting, slicing, composite buffers, working with 
direct and heap memory. By looking at the DrillBuf.java git history, the 
DrillBuf was introduced in 2014 and prior to that Netty classes were 
used directly. Unfortunately, the commit that introduced DrillBuf does 
not provide any info why it was introduced and does not have a reference 
to a JIRA.


One may argue that DrillBuf is a way for Drill to encapsulate Netty 
ByteBuf and guard other modules that use DrillBuf from Netty ByteBuf 
API, so if Netty decides to change ByteBuf API in the next major 
release, amount of changes will be limited to DrillBuf only. Problem is 
that DrillBuf inherits from Netty AbstractByteBuf, so the above goal is 
not achieved either.




1. Boundary checking (on/off based on a flag or assertions being 
enabled/disabled, always on, always off, any other suggestions)

By understanding the design, we can see that we do, in fact, need both checked 
and unchecked methods. The row set mechanisms takes it upon themselves to have 
sufficient context to ensure that memory access is always within bounds, and so 
can benefit from the unchecked methods. As we said, we need debug-time bounds 
checks to catch errors during development.

On the other hand, value vectors should probably be protected by using checked 
methods because they do not have intrinsic mechanisms that ensure correct 
access. With vectors, the memory location to access is set by the caller (each 
operator) and there is no guarantee that all this code is correct all the time. 
(Though, it probably is right now because if it wasn't we'd get errors.)
I don't see a difference in bounds checking requirements between row set 
mechanism and value vectors as value vectors do have "safe" methods or 
intrinsic mechanism that ensures correct access. If not all operators 
use "safe" methods, than that operator should provide a guarantee. At 
the end, if an operator accesses memory out of allocated bounds, 
boundary checking will not fix it. If there is a bug in row set 
mechanism, value vectors or an operator, the end result (crash of the 
JVM) is the same for all.


The proposal just made represents a change; currently the two mechanisms use the same set 
of methods, which puts us into the "should we turn on bounds checks for everyone or 
turn them off for everyone" dilemma.

This is a technical design decision, not a community preference (other than 
that we'd prefer that stuff works...)
On the dev list, almost everything is a technical design decision and 
the community (compared to Drill users who prefer that the community 
makes a choice and stuff works) needs to agree on how to proceed and 
what development practices to adopt and follow.



2. Ref count checking (delegate to netty or have a separate mechanism to 
enable/disable, always on or off)


Ref counts are absolutely necessary, in the current design, for the reasons 
explained earlier: a single memory block can be shared by multiple DrillBufs. 
We have no other way at present to know when the last reference goes away if we 
don't have ref counts.


To deprecate reference counts, we'd have to rework the way that memory is 
transferred between operators. We'd have to deprecate shared buffers. (Or, we'd 
have to move to the fixed blocks mentioned earlier; but even then we'd need ref 
counts if a single sender can feed data to multiple fragments without copies.)

Again, this is not a preference issue, it is a fundamental design issue (unless 
you know of a trick to remove the need for ref counts, in which case please do 
propose it.) Or, if there is a better way to implement bounds checks that is 
faster or simpler, please do propose that.
The question is not about deprecating reference count mechanism. 
Question is whether or not to check that reference count is not zero 
every time DrillBuf is used (see ensureAccessible()).




3. Usage of UDLE


If we meet the design goals stated earlier, and DrillBuf is the only 

Re: [DISCUSS] DrillBuf

[Paul Rogers]

Hi Vlad,
>  I'd suggest to keep focus on DrillBuf design and implementation as the only 
> gate for accessing raw (direct) memory.

I was doing that. By explaining where DrillBuf fits in the overall design, we 
see that DrillBuf should be the only access point for direct memory. The 
context explains why this is the right decision. Changes to DrillBuf should 
support our design as DrillBuf only exists for that purpose.


> 1. Boundary checking (on/off based on a flag or assertions being 
> enabled/disabled, always on, always off, any other suggestions)

By understanding the design, we can see that we do, in fact, need both checked 
and unchecked methods. The row set mechanisms takes it upon themselves to have 
sufficient context to ensure that memory access is always within bounds, and so 
can benefit from the unchecked methods. As we said, we need debug-time bounds 
checks to catch errors during development.

On the other hand, value vectors should probably be protected by using checked 
methods because they do not have intrinsic mechanisms that ensure correct 
access. With vectors, the memory location to access is set by the caller (each 
operator) and there is no guarantee that all this code is correct all the time. 
(Though, it probably is right now because if it wasn't we'd get errors.)

The proposal just made represents a change; currently the two mechanisms use 
the same set of methods, which puts us into the "should we turn on bounds 
checks for everyone or turn them off for everyone" dilemma.

This is a technical design decision, not a community preference (other than 
that we'd prefer that stuff works...)

> 2. Ref count checking (delegate to netty or have a separate mechanism to 
> enable/disable, always on or off)


Ref counts are absolutely necessary, in the current design, for the reasons 
explained earlier: a single memory block can be shared by multiple DrillBufs. 
We have no other way at present to know when the last reference goes away if we 
don't have ref counts.


To deprecate reference counts, we'd have to rework the way that memory is 
transferred between operators. We'd have to deprecate shared buffers. (Or, we'd 
have to move to the fixed blocks mentioned earlier; but even then we'd need ref 
counts if a single sender can feed data to multiple fragments without copies.)

Again, this is not a preference issue, it is a fundamental design issue (unless 
you know of a trick to remove the need for ref counts, in which case please do 
propose it.) Or, if there is a better way to implement bounds checks that is 
faster or simpler, please do propose that.


> 3. Usage of UDLE


If we meet the design goals stated earlier, and DrillBuf is the only interface 
to memory, then we could change the internal representation if there were a 
good reason to do so. Since you listed UDLE, you probably have an idea in mind. 
To make the code simpler? Faster? Perhaps make a proposal.

> 4. Changing DrillBuf to follow Netty convention

Why? Vectors are not byte buffers; they just happen to use them as explained 
earlier. Vectors are actually, well, vectors of fixed types with rigid 
structure, so DrillBuf should provide methods that implement that design. 
Further, Netty never uses DrillBuf so who would benefit from an API change? 
Just to make the code cleaner? To gain performance? Something else? Again, 
proposal? Note that the comment about checked and unchecked methods made 
earlier suggests we'd make DrillBuf even *less* like Netty...


5. Moving DrillBuf to a different package


This is purely a functional question. As the lore has it, DrillBuf is in the 
Netty package because it needs (or needed) visibility to protected members in 
Netty classes. If that is no longer true, then lets do move it to the Drill 
package so we confuse ourselves less about what is and what is not Drill code. 
(I'm STILL confused...) Have you checked to see if things compile if we do the 
move?


Thanks,

- Paul

 

On Thursday, April 5, 2018, 10:09:18 AM PDT, Vlad Rozov  
wrote:  
 
 Hi Paul,

Thank you, it is good to have a different angle view on Vectors, 
Operators and Batches. For this thread, I'd suggest to keep focus on 
DrillBuf design and implementation as the only gate for accessing raw 
(direct) memory. It will be good for the community to agree on

1. Boundary checking (on/off based on a flag or assertions being 
enabled/disabled, always on, always off, any other suggestions)
2. Ref count checking (delegate to netty or have a separate mechanism to 
enable/disable, always on or off)
3. Usage of UDLE
4. Changing DrillBuf to follow Netty convention
5. Moving DrillBuf to a different package

Thank you,

Vlad

On 4/4/18 11:18, Paul Rogers wrote:
> Hi Vlad,
>
> Would be great to get insight from the original authors. Here ismy two cents 
> as a late comer who made extensive use of the classes in question.
>
> Many of your questions are at the implementation level. It is worth looking 
> at the 

Re: [DISCUSS] DrillBuf

[Vlad Rozov]

Hi Paul,

Thank you, it is good to have a different angle view on Vectors, 
Operators and Batches. For this thread, I'd suggest to keep focus on 
DrillBuf design and implementation as the only gate for accessing raw 
(direct) memory. It will be good for the community to agree on


1. Boundary checking (on/off based on a flag or assertions being 
enabled/disabled, always on, always off, any other suggestions)
2. Ref count checking (delegate to netty or have a separate mechanism to 
enable/disable, always on or off)

3. Usage of UDLE
4. Changing DrillBuf to follow Netty convention
5. Moving DrillBuf to a different package

Thank you,

Vlad

On 4/4/18 11:18, Paul Rogers wrote:

Hi Vlad,

Would be great to get insight from the original authors. Here ismy two cents as 
a late comer who made extensive use of the classes in question.

Many of your questions are at the implementation level. It is worth looking at 
the question from two other perspectives: history and design.

Historically, Drill adopted Netty for networking, and wisely looked for ways of 
using the same buffers for both network transfer and internal operations to 
avoid copies. Some overview is in [1]. In this view, a Drill vector is a 
network buffer. Network buffers use the ByteBuffer protocol to serialize binary 
values. DrillBuf follows that model for the most part. Because a ByteBuffer is 
a low-level abstraction over a buffer, each operation must perform bounds 
checks to ensure safe operation.


DrillBuf provides the ability to present a "view" of a slice of a larger underlying 
buffer. For example, when reading data from a spill file, all data for all internal vectors is read 
into a single buffer. For a nullable VarChar, for example, the buffer contains the bit vectors, the 
offset vectors and the data vectors. The value vectors point to DrillBufs which point to a slice of 
the underlying buffers. It is this layout (there are at least three different layouts) that makes 
our "record batch sizer" so complex: the size of memory used is NOT the sum of the 
DrillBufs.

Drill is a columnar system. So, the team introduced a typed "vector" 
abstraction.Value vectors provide an abstraction that sweeps away the ByteBuffer heritage 
and replaces it with a strongly typed, accessor/mutator structure that works in terms of 
Drill data types and record counts. Vectors also understand the relationship between bit 
vectors and data vectors, between offset vectors and data vectors, and so on.

Your question implies a desire to think about the future direction. Two things 
to say. First, vectors themselves do not provide sufficient abstraction for the 
needs of operators. As a result, operators become very complex, we must 
generate large amounts of boiler-plate code, and we fix the same bugs over and 
over. These issues are discussed at length in [2]. This is the motivation for 
the result set reader and loader.

The row set abstractions encapsulate not just knowledge of a vector, but of the 
entire batch. As a result, these abstractions know the number of records, know 
the vector and batch size targets, and track vectors as they fill. One key 
result is that these abstractions ensure that data is read or written within 
the bounds of each buffer, eliminating the need for bounds checks on every 
access.


The other consideration is memory management. Drill has a very complex, but surprisingly 
robust, memory management system. However, it is based on a "malloc" model of 
memory with operators negotiating among themselves (via the OUT_OF_MEMORY iterator 
status) about who needs memory and who should release it. [2] discusses the limitations 
of this system. As a result, we've been moving to a budget-based system in which each 
fragment and operator is given a budget based on total available memory, and operators 
use spilling to stay within the budget.

Memory fragmentation is a classic problem in malloc-based systems which strive 
to operate at high memory utilization rates and which do not include memory 
compaction. Drill is such a system. So, if this issue ever prevents Drill from 
achieving maximum performance, we can consider the classic system used by 
databases to solve this problem: fixed-size memory blocks.

If we were to move to fixed-size buffers, we'd want the row set and vector 
abstractions to remain unchanged. We'd only want to replace DrillBuf with a new 
block-based abstraction, perhaps with chaining (a vector may consist of a chain 
of, say, 1 MB blocks.) The buffer slicing mechanism would become unnecessary, 
as would the existing malloc-based allocator. Instead, data would be read, 
written and held in buffers allocated from and returned to a buffer pool.


We may or may not ever make such a change. But, by considering this 
possibility, we readily see that DrillBuf should be an implementation detail of 
the higher-level abstractions and that operators should only use those 
higher-level abstractions because doing so isolates 

Re: [DISCUSS] DrillBuf

[Paul Rogers]

Hi Vlad,

Would be great to get insight from the original authors. Here ismy two cents as 
a late comer who made extensive use of the classes in question.

Many of your questions are at the implementation level. It is worth looking at 
the question from two other perspectives: history and design.

Historically, Drill adopted Netty for networking, and wisely looked for ways of 
using the same buffers for both network transfer and internal operations to 
avoid copies. Some overview is in [1]. In this view, a Drill vector is a 
network buffer. Network buffers use the ByteBuffer protocol to serialize binary 
values. DrillBuf follows that model for the most part. Because a ByteBuffer is 
a low-level abstraction over a buffer, each operation must perform bounds 
checks to ensure safe operation.


DrillBuf provides the ability to present a "view" of a slice of a larger 
underlying buffer. For example, when reading data from a spill file, all data 
for all internal vectors is read into a single buffer. For a nullable VarChar, 
for example, the buffer contains the bit vectors, the offset vectors and the 
data vectors. The value vectors point to DrillBufs which point to a slice of 
the underlying buffers. It is this layout (there are at least three different 
layouts) that makes our "record batch sizer" so complex: the size of memory 
used is NOT the sum of the DrillBufs.

Drill is a columnar system. So, the team introduced a typed "vector" 
abstraction.Value vectors provide an abstraction that sweeps away the 
ByteBuffer heritage and replaces it with a strongly typed, accessor/mutator 
structure that works in terms of Drill data types and record counts. Vectors 
also understand the relationship between bit vectors and data vectors, between 
offset vectors and data vectors, and so on.

Your question implies a desire to think about the future direction. Two things 
to say. First, vectors themselves do not provide sufficient abstraction for the 
needs of operators. As a result, operators become very complex, we must 
generate large amounts of boiler-plate code, and we fix the same bugs over and 
over. These issues are discussed at length in [2]. This is the motivation for 
the result set reader and loader.

The row set abstractions encapsulate not just knowledge of a vector, but of the 
entire batch. As a result, these abstractions know the number of records, know 
the vector and batch size targets, and track vectors as they fill. One key 
result is that these abstractions ensure that data is read or written within 
the bounds of each buffer, eliminating the need for bounds checks on every 
access.


The other consideration is memory management. Drill has a very complex, but 
surprisingly robust, memory management system. However, it is based on a 
"malloc" model of memory with operators negotiating among themselves (via the 
OUT_OF_MEMORY iterator status) about who needs memory and who should release 
it. [2] discusses the limitations of this system. As a result, we've been 
moving to a budget-based system in which each fragment and operator is given a 
budget based on total available memory, and operators use spilling to stay 
within the budget.

Memory fragmentation is a classic problem in malloc-based systems which strive 
to operate at high memory utilization rates and which do not include memory 
compaction. Drill is such a system. So, if this issue ever prevents Drill from 
achieving maximum performance, we can consider the classic system used by 
databases to solve this problem: fixed-size memory blocks.

If we were to move to fixed-size buffers, we'd want the row set and vector 
abstractions to remain unchanged. We'd only want to replace DrillBuf with a new 
block-based abstraction, perhaps with chaining (a vector may consist of a chain 
of, say, 1 MB blocks.) The buffer slicing mechanism would become unnecessary, 
as would the existing malloc-based allocator. Instead, data would be read, 
written and held in buffers allocated from and returned to a buffer pool.


We may or may not ever make such a change. But, by considering this 
possibility, we readily see that DrillBuf should be an implementation detail of 
the higher-level abstractions and that operators should only use those 
higher-level abstractions because doing so isolates operators from the details 
of memory layout. This argument applies even more so to the abstractions below 
DrillBuf: UDLE, Netty ByteBuf, ledgers and so on.

Said another way, even with the current system, we should be free to improve 
DrillBuf on down with no impact to operator code because vectors and the row 
set abstractions should be the only clients of DrillBuf.


In short, by understanding the history of the code, and agreeing upon the right 
design abstractions, we can then make informed decisions about how best to 
improve our low-level abstractions, including DrillBuf.


Thanks,

- Paul

[1] http://drill.apache.org/docs/value-vectors/
[2] 

[DISCUSS] DrillBuf

[Vlad Rozov]

I have several questions and concerns regarding DrillBuf usage, design 
and implementation. There is a limited documentation available for the 
subject (Java doc, 
https://github.com/apache/drill/blob/master/exec/memory/base/src/main/java/org/apache/drill/exec/memory/README.md 
and https://github.com/paul-rogers/drill/wiki/Memory-Management) and I 
hope that a few members of the community may have more information.


What are the design goals behind DrillBuf? It seems like it is supposed 
to be Drill access gate for direct byte buffers. How is it different 
(for that goal) from UnsafeDirectLittleEndian? Both use 
wrapper/delegation pattern, with DrillBuf delegating to 
UnsafeDirectLittleEndian (not always) and UnsafeDirectLittleEndian 
delegating to ByteBuf it wraps. Is it necessary to have both? Are there 
any out of the box netty classes that already provide required 
functionality? I guess that answer to the last question was "no" back 
when DrillBuf and UnsafeDirectLittleEndian were introduced into Drill. 
Is it still "no" for the latest netty release? What extra functionality 
DrillBuf (and UnsafeDirectLittleEndian) provides on top of existing 
netty classes?


As far as I can see from the source code, DrillBuf changes validation 
(boundary and reference count checks) mechanism, making it optional 
(compared to always enabled boundary checks inside netty) for get/set 
Byte/Char/Short/Long/Float/Double. Is this a proper place to make 
validation optional or the validation (or portion of the validation) 
must be always on or off (there are different opinions, see 
https://issues.apache.org/jira/browse/DRILL-6004, 
https://issues.apache.org/jira/browse/DRILL-6202, 
https://github.com/apache/drill/pull/1060 and 
https://github.com/apache/drill/pull/1144)? Are there any performance 
benchmark that justify or explain such behavior (if such benchmark does 
not exist, are there any volunteer to do the benchmark)? My experience 
is that the reference count check is significantly more expensive 
compared to boundary checking and boundary checking adds tens of percent 
to direct memory read when reading just a few bytes, so my vote is to 
keep validation as optional with the ability to enable it for debug 
purposes at run-time. What is the reason the same approach do not apply 
to get/set Bytes and those methods are delegated to 
UnsafeDirectLittleEndian that delegates it further?


Why DrillBuf reverses how AbstractByteBuf calls _get from get (and _set 
from set), making _get to call get (_set to call set)? Why not to follow 
a base class design patter?


Another question is usage of netty "io.netty.buffer" package for Drill 
classes. Is this absolutely necessary? I don't think that netty 
developers expect this and support semantic version compatibility for 
package private classes/members.


Thank you,

Vlad