Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Feb 7, 2020

What changes were proposed in this pull request?

In the case of back-to-back calculation of floorDiv and floorMod with the same arguments, the result of foorDiv can be reused in calculation of floorMod. The floorMod method is defined as the following in Java standard library:

    public static int floorMod(int x, int y) {
        int r = x - floorDiv(x, y) * y;
        return r;
    }

If floorDiv(x, y) has been already calculated, it can be reused in x - floorDiv(x, y) * y.

I propose to modify 2 places in DateTimeUtils:

  1. microsToInstant which is widely used in many date-time functions. Math.floorMod(us, MICROS_PER_SECOND) is just replaced by its definition from Java Math library.
  2. truncDate: Math.floorMod(oldYear, divider) == 0 is replaced by Math.floorDiv(oldYear, divider) * divider == oldYear where floorDiv(...) * divider is pre-calculated.

Why are the changes needed?

This reduces the number of arithmetic operations, and can slightly improve performance of date-time functions.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By existing test suites DateTimeUtilsSuite, DateFunctionsSuite and DateExpressionsSuite.

@MaxGekk MaxGekk changed the title [MINOR] Reuse results of floorDiv in calculations of floorMod in DateTimeUtils [SPARK-30754][SQL] Reuse results of floorDiv in calculations of floorMod in DateTimeUtils Feb 7, 2020
@SparkQA
Copy link

SparkQA commented Feb 7, 2020

Test build #118031 has finished for PR 27491 at commit 363d8a2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Feb 7, 2020

It's pretty trivially faster, I'd imagine, but the logic looks sound and it's not more complex

@kiszk
Copy link
Member

kiszk commented Feb 9, 2020

Can we use a kind of this optimization in HotSpot?

@MaxGekk
Copy link
Member Author

MaxGekk commented Feb 9, 2020

@kiszk The ops are not regular arithmetic / and %, they are lib calls. So, they present in byte code:

public microsToInstant(J)Ljava/time/Instant;
    // parameter final  us
   L0
    LINENUMBER 339 L0
    LLOAD 1
    LDC 1000000
    INVOKESTATIC java/lang/Math.floorDiv (JJ)J
   L1
    LSTORE 3
   L2
    LINENUMBER 340 L2
    LLOAD 1
    LDC 1000000
    INVOKESTATIC java/lang/Math.floorMod (JJ)J

@maropu
Copy link
Member

maropu commented Feb 9, 2020

The fix looks fine to me. If you have performance numbers, could you put them in the PR decription?

@kiszk
Copy link
Member

kiszk commented Feb 10, 2020

@MaxGekk Got it. This is not a pair of div and mod.

@SparkQA
Copy link

SparkQA commented Feb 10, 2020

Test build #118122 has finished for PR 27491 at commit 38cd6da.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@MaxGekk
Copy link
Member Author

MaxGekk commented Feb 10, 2020

jenkins, retest this, please

Copy link
Member

@maropu maropu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. I'll leave this to @srowen and the other committers. cc: @dongjoon-hyun @HyukjinKwon

@SparkQA
Copy link

SparkQA commented Feb 10, 2020

Test build #118140 has finished for PR 27491 at commit 38cd6da.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Feb 11, 2020

Merged to master (leaving out of 3.0)

@srowen srowen closed this in dc66d57 Feb 11, 2020
sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
…Mod in DateTimeUtils

### What changes were proposed in this pull request?
In the case of back-to-back calculation of `floorDiv` and `floorMod` with the same arguments, the result of `foorDiv` can be reused in calculation of `floorMod`. The `floorMod` method is defined as the following in Java standard library:
```java
    public static int floorMod(int x, int y) {
        int r = x - floorDiv(x, y) * y;
        return r;
    }
```
If `floorDiv(x, y)` has been already calculated, it can be reused in `x - floorDiv(x, y) * y`.

I propose to modify 2 places in `DateTimeUtils`:
1. `microsToInstant` which is widely used in many date-time functions. `Math.floorMod(us, MICROS_PER_SECOND)` is just replaced by its definition from Java Math library.
2. `truncDate`: `Math.floorMod(oldYear, divider) == 0` is replaced by `Math.floorDiv(oldYear, divider) * divider == oldYear` where `floorDiv(...) * divider` is pre-calculated.

### Why are the changes needed?
This reduces the number of arithmetic operations, and can slightly improve performance of date-time functions.

### Does this PR introduce any user-facing change?
No

### How was this patch tested?
By existing test suites `DateTimeUtilsSuite`, `DateFunctionsSuite` and `DateExpressionsSuite`.

Closes apache#27491 from MaxGekk/opt-microsToInstant.

Authored-by: Maxim Gekk <max.gekk@gmail.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
@MaxGekk MaxGekk deleted the opt-microsToInstant branch June 5, 2020 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants