[ 
https://issues.apache.org/jira/browse/DRILL-5122?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Paul Rogers updated DRILL-5122:
-------------------------------
    Description: 
The {{DrillBuf}} class is the foundation of Drill's direct memory model. It has 
a very sophisticated allocator mechanism behind it. {{DrillBuf}} provides 
logging to help track down memory leaks and other issues. Unfortunately, the 
trigger to turn on this option is based on detecting if assertions are enabled 
in the JVM.

Assertions are very handy tools: they are debug-only assertions not executed at 
run time. They are turned on with the "-ea" option to the JVM. But, in 
production, the assertions don't execute, avoiding production-time costs.

In Drill, once assertions are enabled, the dominant cost of a query becomes 
{{DrillBuf}} logging. One example showed that {{DrillBuf}} logging increases 
query cost by 10x.

Debugging of {{DrillBuf}} is handy for those few engineers who need to do so. 
Over time, {{DrillBuf}} has become quite stable so that, now, most developers, 
most of the time, do not work on {{DrillBuf}}.

One wonders: Drill uses Guava preconditions extensively. Is this perhaps due to 
the high cost of enabling assertions? Since assertions were unavailable, 
production-time checking via Guava may have become preferred. Assertions are, 
in fact, better because they incur no overhead in production, while Guava 
preconditions always execute, even in production. (Here we assume that the code 
errors that both check will find errors during development so that such errors 
do not sneak through into production.)

The code does have a way to turn off debugging, but only if a system property 
is set on every run. Since Drill has hundreds of unit tests that people might 
run, this requires creating custom run configurations for each.

This enhancement proposes to modify {{DrillBuf}} to enable logging only when a) 
assertions are enabled (so we know it is a debug environment) and b) when a 
boot-time configuration property is set (so that logging can be turned off for 
most developers most of the time.) Optionally, b) can be a system property that 
_enables_ (not disables) {{DrillBuf}} logging.

The code in question:
{code}
class BaseAllocator ... {
  public static final boolean DEBUG = AssertionUtil.isAssertionsEnabled()
      || Boolean.parseBoolean(System.getProperty(DEBUG_ALLOCATOR, "false"));
...
class DrillBuf ... {
  public boolean release(int decrement) {
    ...
    if (BaseAllocator.DEBUG) {
      historicalLog.recordEvent("release(%d). original value: %d", decrement, 
refCnt + decrement);
    }
{code}

  was:
The {{DrillBuf}} class is the foundation of Drill's direct memory model. It has 
a very sophisticated allocator mechanism behind it. {{DrillBuf}} provides 
logging to help track down memory leaks and other issues. Unfortunately, the 
trigger to turn on this option is based on detecting if assertions are enabled 
in the JVM.

Assertions are very handy tools: they are debug-only assertions not executed at 
run time. They are turned on with the "-ea" option to the JVM. But, in 
production, the assertions don't execute, avoiding production-time costs.

In Drill, once assertions are enabled, the dominant cost of a query becomes 
{{DrillBuf}} logging. On example showed that {{DrillBuf}} logging increases 
query cost by 10x.

Debugging of {{DrillBuf}} is handy for those few engineers who need to do so. 
Over time, {{DrillBuf}} has become quite stable so that, now, most developers, 
most of the time, do not work on {{DrillBuf}}.

One wonders: Drill uses Guava preconditions extensively. Is this perhaps due to 
the high cost of enabling assertions? Since assertions were unavailable, 
production-time checking via Guava may have become preferred. Assertions are, 
in fact, better because they incur no overhead in production, while Guava 
preconditions always execute, even in production. (Here we assume that the code 
errors that both check will find errors during development so that such errors 
do not sneak through into production.)

The code does have a way to turn off debugging, but only if a system property 
is set on every run. Since Drill has hundreds of unit tests that people might 
run, this requires creating custom run configurations for each.

This enhancement proposes to modify {{DrillBuf}} to enable logging only when a) 
assertions are enabled (so we know it is a debug environment) and b) when a 
boot-time configuration property is set (so that logging can be turned off for 
most developers most of the time.) Optionally, b) can be a system property that 
_enables_ (not disables) {{DrillBuf}} logging.

The code in question:
{code}
class BaseAllocator ... {
  public static final boolean DEBUG = AssertionUtil.isAssertionsEnabled()
      || Boolean.parseBoolean(System.getProperty(DEBUG_ALLOCATOR, "false"));
...
class DrillBuf ... {
  public boolean release(int decrement) {
    ...
    if (BaseAllocator.DEBUG) {
      historicalLog.recordEvent("release(%d). original value: %d", decrement, 
refCnt + decrement);
    }
{code}


> DrillBuf performs very expensive logging if JVM -ea option is set
> -----------------------------------------------------------------
>
>                 Key: DRILL-5122
>                 URL: https://issues.apache.org/jira/browse/DRILL-5122
>             Project: Apache Drill
>          Issue Type: Improvement
>          Components:  Server
>    Affects Versions: 1.8.0
>            Reporter: Paul Rogers
>            Assignee: Paul Rogers
>            Priority: Minor
>
> The {{DrillBuf}} class is the foundation of Drill's direct memory model. It 
> has a very sophisticated allocator mechanism behind it. {{DrillBuf}} provides 
> logging to help track down memory leaks and other issues. Unfortunately, the 
> trigger to turn on this option is based on detecting if assertions are 
> enabled in the JVM.
> Assertions are very handy tools: they are debug-only assertions not executed 
> at run time. They are turned on with the "-ea" option to the JVM. But, in 
> production, the assertions don't execute, avoiding production-time costs.
> In Drill, once assertions are enabled, the dominant cost of a query becomes 
> {{DrillBuf}} logging. One example showed that {{DrillBuf}} logging increases 
> query cost by 10x.
> Debugging of {{DrillBuf}} is handy for those few engineers who need to do so. 
> Over time, {{DrillBuf}} has become quite stable so that, now, most 
> developers, most of the time, do not work on {{DrillBuf}}.
> One wonders: Drill uses Guava preconditions extensively. Is this perhaps due 
> to the high cost of enabling assertions? Since assertions were unavailable, 
> production-time checking via Guava may have become preferred. Assertions are, 
> in fact, better because they incur no overhead in production, while Guava 
> preconditions always execute, even in production. (Here we assume that the 
> code errors that both check will find errors during development so that such 
> errors do not sneak through into production.)
> The code does have a way to turn off debugging, but only if a system property 
> is set on every run. Since Drill has hundreds of unit tests that people might 
> run, this requires creating custom run configurations for each.
> This enhancement proposes to modify {{DrillBuf}} to enable logging only when 
> a) assertions are enabled (so we know it is a debug environment) and b) when 
> a boot-time configuration property is set (so that logging can be turned off 
> for most developers most of the time.) Optionally, b) can be a system 
> property that _enables_ (not disables) {{DrillBuf}} logging.
> The code in question:
> {code}
> class BaseAllocator ... {
>   public static final boolean DEBUG = AssertionUtil.isAssertionsEnabled()
>       || Boolean.parseBoolean(System.getProperty(DEBUG_ALLOCATOR, "false"));
> ...
> class DrillBuf ... {
>   public boolean release(int decrement) {
>     ...
>     if (BaseAllocator.DEBUG) {
>       historicalLog.recordEvent("release(%d). original value: %d", decrement, 
> refCnt + decrement);
>     }
> {code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to