Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201706062033.v56KXpmA029208@ip-10-146-233-104.ec2.internal>
Date: Tue, 6 Jun 2017 20:33:51 +0000
From: "Impala Public Jenkins (Code Review)" <gerrit@cloudera.org>
To: Matthew Jacobs <mj@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5357=3A_Fix_unixtime_to_UTC_TimestampValue_perf=0A?=
In-Reply-To: <gerrit.1496690986000.I9d611d21310c7b4f93d8e0bc845eb85125abcac9@gerrit.cloudera.org>
References: <gerrit.1496690986000.I9d611d21310c7b4f93d8e0bc845eb85125abcac9@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Tue, 06 Jun 2017 20:34:03 -0000

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5357: Fix unixtime to UTC TimestampValue perf
......................................................................


IMPALA-5357: Fix unixtime to UTC TimestampValue perf

Fixes the severe perf issue when calling gmtime_r to convert
a unix time because that libc function takes a global lock.
Instead, use boost ptime::from_time_t when possible.
Unfortunately only a range of input times are supported
without overflowing the underlying time_duration struct
(dates between 1677-2262), so those dates outside that range
are handled with the slow path calling into gmtime_r.

This requires using a patched build of boost which
includes an upstream fix for the time_duration class, see:
https://github.com/cloudera/native-toolchain/commit/6e726b4b8164d53814f75b78a681fcf6df4a887a

Testing:
* Ran an exhaustive test run successfully.
* Wrote a test program to validate that all time_t values
  between 1677 and 2262 are converted to the same ptime
  when using the new code path and the old path. Doing so
  required running all night, so I'm not going to attempt to
  add this test. I think a single validation is acceptable.

Perf impact:
Microbenchmark of the new path (conversion using boost) and
the old path (conversion using libc gmtime_r) resulted in
the expected speedup from not using a global lock.

Machine Info: Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Function    10%ile   50%ile   90%ile     10%ile     50%ile     90%ile
          iters/ms iters/ms iters/ms (relative) (relative) (relative)
---------------------------------------------------------------------
  libc-1     0.192    0.196      0.2         1X         1X         1X
 boost-1     0.333     0.34     0.34      1.73X      1.73X       1.7X
  libc-2     0.112    0.115    0.116     0.584X     0.586X     0.581X
 boost-2     0.627    0.654    0.667      3.26X      3.33X      3.33X
  libc-4     0.042   0.0478   0.0515     0.218X     0.244X     0.258X
 boost-4     0.863     1.27      1.3      4.49X       6.5X       6.5X
  libc-8    0.0326   0.0328   0.0329     0.169X     0.167X     0.164X
 boost-8     0.741    0.902      1.1      3.85X       4.6X       5.5X

Change-Id: I9d611d21310c7b4f93d8e0bc845eb85125abcac9
Reviewed-on: http://gerrit.cloudera.org:8080/7082
Reviewed-by: Matthew Jacobs <mj@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/timestamp-test.cc
M be/src/runtime/timestamp-value.cc
2 files changed, 66 insertions(+), 13 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Matthew Jacobs: Looks good to me, approved


-- 
To view, visit http://gerrit.cloudera.org:8080/7082
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I9d611d21310c7b4f93d8e0bc845eb85125abcac9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>