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.

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

Cache TableRecord Constructor in TableImpl for use in Tools.recordFactory() #6639

Closed
lukaseder opened this issue Oct 5, 2017 · 7 comments
Closed

Comments

@lukaseder
Copy link
Member

Currently, when creating TableRecord in result queries, the record is instanciated using reflection, which allows for creating records from private constructors, too.

However, this constructor is looked up every single time, instead of it being cached:

final Constructor<R> constructor = Reflect.accessible(type.getDeclaredConstructor());

In a profiling session running 1M queries, this accounts for 0.85% of the samples, so it's definitely worth optimising:

image

@lukaseder
Copy link
Member Author

This may as well have been a safepoint artifact from the profiling session. I've never noticed this again since.

3.19 Other improvements automation moved this from To do to Done Aug 9, 2023
@lukaseder lukaseder reopened this Feb 9, 2024
@lukaseder
Copy link
Member Author

This did show up again in another profiling session, as one of the primary allocation problems:

image

A quick test reverting type.getDeclaredConstructor().newInstance() to type.newInstance() (deprecated, but cached) yielded promising results. I'll investigate this again.

@lukaseder
Copy link
Member Author

Ah, the difference is that in the original screenshot, the recordFactory() call was done per-cursor, which isn't really worth caching.

But now we're looking into a benchmark where Result.into(Table) is called, and that possibly multiple times. Or even users looping over Result and calling Record.into(Table) on individual records.

Perhaps, a generated TableImpl could have a cached reference to its generated TableRecord constructor, if available.

@lukaseder
Copy link
Member Author

I've tried caching the Constructor in the Configuration, but that's slower than the status quo.

@lukaseder
Copy link
Member Author

Attaching the cached Constructor to TableImpl (i.e. the generated tables) produces good results:

Before

BenchmarkRecordMapper.intoTableCache          thrpt    7  3134634.762 ┬▒  55709.662  ops/s
BenchmarkRecordMapper.intoTableNoCache        thrpt    7  3126145.774 ┬▒ 105931.506  ops/s

After

BenchmarkRecordMapper.intoTableCache          thrpt    7  3369384.579 ┬▒ 39552.058  ops/s
BenchmarkRecordMapper.intoTableNoCache        thrpt    7  3378989.655 ┬▒ 54101.392  ops/s

Roughly 5% better on Result.into(Table) calls is definitely worth it as this also affects all kinds of Result.intoGroups(Table, Table) and other similar usages. CursorImpl will also profit, but only marginally, because it looks up the constructor only once per CursorImpl.

@lukaseder
Copy link
Member Author

In fact, this improves any usage of DSLContext.newRecord(Table) and related usages where a Table is available to construct a TableRecord

@lukaseder lukaseder changed the title Cache Constructor in RecordFactory Cache TableRecord Constructor in TableImpl for use in Tools.recordFactory() Feb 9, 2024
@lukaseder lukaseder added this to To do in 3.20 Other improvements via automation Feb 9, 2024
lukaseder added a commit that referenced this issue Feb 9, 2024
lukaseder added a commit that referenced this issue Feb 9, 2024
lukaseder added a commit that referenced this issue Feb 9, 2024
lukaseder added a commit that referenced this issue Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

1 participant