[SPARK-14092] [SQL] move shouldStop() to end of while loop

[davies]

What changes were proposed in this pull request?

This PR rollback some changes in #11274 , which introduced some performance regression when do a simple aggregation on parquet scan with one integer column.

Does not really understand how this change introduce this huge impact, maybe related show JIT compiler inline functions. (saw very different stats from profiling).

How was this patch tested?

Manually run the parquet reader benchmark, before this change:

Intel(R) Core(TM) i7-4558U CPU @ 2.80GHz
Int and String Scan:                Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
-------------------------------------------------------------------------------------------
SQL Parquet Vectorized                   2391 / 3107         43.9          22.8       1.0X

After this change

Java HotSpot(TM) 64-Bit Server VM 1.7.0_60-b19 on Mac OS X 10.9.5
Intel(R) Core(TM) i7-4558U CPU @ 2.80GHz
Int and String Scan:                Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
-------------------------------------------------------------------------------------------
SQL Parquet Vectorized                   2032 / 2626         51.6          19.4       1.0X```

[rxin]

We would need to dump the JITed assembly to understand what's going on.

[rxin]

Can we add some comment somewhere to explain why shouldStop needs to be here? It'd be great to reference the JIRA ticket.

[davies]

It's here in the beginning.

[rxin]

where is it?

[rxin]

btw i'm not sure but i suspect this has to do with loop unrolling. jit stops unrolling the loop when shouldStop is part of the terminal condition.

[nongli]

Can we add a comment around line 248 saying this loop is very perf sensitive and changes to it should be measured carefully?

[SparkQA]

Test build #53905 has finished for PR 11912 at commit b31115d.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • final class GeneratedIterator extends org.apache.spark.sql.execution.BufferedRowIterator

[nongli]

LGTM

[davies]

Added comment, merging this into master.