Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cache short decimal type instances #14523

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Cache short decimal type instances #14523

merged 1 commit into from
Oct 10, 2022

Conversation

findepi
Copy link
Member

@findepi findepi commented Oct 7, 2022

When profiling Iceberg planning on TPC-DS data set, it was found out that 10% of time spend inside TableStatisticsMaker is actually spent inside ShortDecimalType constructor.

image

@cla-bot cla-bot bot added the cla-signed label Oct 7, 2022
@findepi
Copy link
Member Author

findepi commented Oct 7, 2022

When profiling Iceberg planning on TPC-DS data set

I know this isn't very representative, as TPC-DS tables aren't very wide and my data set isn't comprised from a large number of files. Still, it's kind of surprising to see this show up in a profile, among code that does some IO.

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Oct 7, 2022
@martint
Copy link
Member

martint commented Oct 7, 2022

it was found out that 10% of time spend inside

Most of that time is in the constructor of TypeSignature, and my guess is that it is related to all the validation and list processing.

Caching is an ok workaround for now, but it doesn't solve the underlying problem (which is to fix/get rid of TypeSignature)

@findepi
Copy link
Member Author

findepi commented Oct 7, 2022

https://github.com/trinodb/trino/actions/runs/3205780736/jobs/5240103859 fail may be not related

Error:  io.trino.server.remotetask.TestHttpRemoteTask.testRejectedExecution  Time elapsed: 4.194 s  <<< FAILURE!
java.lang.AssertionError: expected [REMOTE_TASK_ERROR:65542] but found [GENERIC_INTERNAL_ERROR:65536]
	at org.testng.Assert.fail(Assert.java:94)
	at org.testng.Assert.failNotEquals(Assert.java:513)
	at org.testng.Assert.assertEqualsImpl(Assert.java:135)
	at org.testng.Assert.assertEquals(Assert.java:116)
	at org.testng.Assert.assertEquals(Assert.java:179)
	at io.trino.server.remotetask.TestHttpRemoteTask.runTest(TestHttpRemoteTask.java:398)
	at io.trino.server.remotetask.TestHttpRemoteTask.testRejectedExecution(TestHttpRemoteTask.java:167)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:54)
	at org.testng.internal.InvokeMethodRunnable.run(InvokeMethodRunnable.java:44)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)

will retry

@findepi
Copy link
Member Author

findepi commented Oct 7, 2022

filed #14526

@findepi findepi requested review from martint and removed request for martint October 7, 2022 20:05
martint
martint approved these changes Oct 7, 2022
View reviewed changes
core/trino-spi/src/main/java/io/trino/spi/type/ShortDecimalType.java Outdated
@@ -42,7 +43,23 @@
{
private static final TypeOperatorDeclaration TYPE_OPERATOR_DECLARATION = extractOperatorDeclaration(ShortDecimalType.class, lookup(), long.class);

ShortDecimalType(int precision, int scale)
private static final ShortDecimalType[][] shortDecimalTypes;
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize SHORT_DECIMAL_TYPES. I might just call it CACHE or INSTANCES, though.

@findepi
Cache short decimal type instances
b4b1f37 
When profiling Iceberg planning on TPC-DS data set, it was found out that 10% of
time spend inside `TableStatisticsMaker` is actually spent inside
`ShortDecimalType` constructor.
@findepi findepi force-pushed the findepi/cache-short-decimal-type-instances-a9eca5 branch from ed9cad1 to b4b1f37 Compare October 8, 2022 08:14
@findepi findepi merged commit 96a009e into trinodb:master Oct 10, 2022
@findepi findepi deleted the findepi/cache-short-decimal-type-instances-a9eca5 branch October 10, 2022 09:44
@github-actions github-actions bot added this to the 400 milestone Oct 10, 2022
@colebow colebow mentioned this pull request Oct 11, 2022
Merged
@findepi findepi self-assigned this Oct 14, 2022
@findepi findepi mentioned this pull request Nov 4, 2022
Merged
@findepi findepi mentioned this pull request May 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martint martint martint approved these changes

@raunaqmorarka raunaqmorarka Awaiting requested review from raunaqmorarka

@sopel39 sopel39 Awaiting requested review from sopel39

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees

@findepi findepi

Labels
cla-signed no-release-notes This pull request does not require release notes entry
Milestone
400
Development

Successfully merging this pull request may close these issues.
2 participants
@findepi @martint