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

perf(schema) no deep copy on select on process auto fields #8796

Merged
merged 2 commits into from
May 17, 2022

Conversation

bungle
Copy link
Member

@bungle bungle commented May 12, 2022

Summary

It is inefficient to create deep copies of tables when e.g. looping through database rows (and with records in them we recursively deep copy them). In my testing with uploading some 64k routes with dbless (which calls process auto fields twice), this can cut the time looping the data by 1/4th. It also generates much less garbage.

I searched our code bases where we use "select" context, and could not find anything that might break because of this.

@bungle bungle marked this pull request as ready for review May 12, 2022 19:40
@bungle bungle requested a review from a team as a code owner May 12, 2022 19:40
Copy link
Contributor

@flrgh flrgh left a comment

Choose a reason for hiding this comment

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

Soft 👍 My knowledge of the dao/schema modules does not go very deep, but after looking through some of the code this seems relatively safe/sensible.

One note; the plugin doc string states that a new table is returned. It would be good to update this to reflect the new behavior, that the table is modified in-place for select:

-- @return A new table, with the auto fields containing
-- appropriate updated values.

@bungle
Copy link
Member Author

bungle commented May 13, 2022

Soft 👍 My knowledge of the dao/schema modules does not go very deep, but after looking through some of the code this seems relatively safe/sensible.

One note; the plugin doc string states that a new table is returned. It would be good to update this to reflect the new behavior, that the table is modified in-place for select:

-- @return A new table, with the auto fields containing
-- appropriate updated values.

Yes, last time I tried this there was some tests failing, but it might be because of other changes. But now it seems tests are green. The biggest issue to me is things like (I think we rarely feed this thing with "select" :

https://github.com/Kong/kong/blob/master/kong/runloop/handler.lua#L766

And especially on dbless:
https://github.com/Kong/kong/blob/master/kong/db/strategies/off/init.lua#L181-L184
https://github.com/Kong/kong/blob/master/kong/db/dao/init.lua#L1398-L1402

I have been testing this with some 64k routes, and deep copying tables takes around 1/4th of the rebuild time. I will adjust the description there.

@bungle bungle force-pushed the perf/no-deep-clone-on-read branch from 4da2ddd to 84ea2e9 Compare May 13, 2022 07:24
@bungle bungle changed the title perf(schema) no deep clone on select perf(schema) no deep copy on select on process auto fields May 13, 2022
@bungle bungle requested a review from flrgh May 13, 2022 07:25
@bungle
Copy link
Member Author

bungle commented May 13, 2022

I will adjust the description there.

The description was adjusted and tests were added (so this is now "official", and not undefined behavior).

@bungle bungle force-pushed the perf/no-deep-clone-on-read branch from 84ea2e9 to eee3867 Compare May 13, 2022 07:33
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 13, 2022
@bungle bungle force-pushed the perf/no-deep-clone-on-read branch from 39be720 to 8802f44 Compare May 13, 2022 08:39
Copy link
Member

@gszr gszr left a comment

Choose a reason for hiding this comment

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

LGTM - unless @flrgh has other concerns.

kong/db/strategies/off/init.lua Outdated Show resolved Hide resolved
Copy link
Member

@gszr gszr left a comment

Choose a reason for hiding this comment

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

Failures in this PR (and other perf PRs) are unrelated; as the perf testing framework picks a package for the latest official Kong release, and Kong master requires a newer OpenResty version, the older OpenResty isn't recognizing a new Nginx directive that is being used.

@bungle bungle force-pushed the perf/no-deep-clone-on-read branch from 8802f44 to dba3fef Compare May 17, 2022 09:27
bungle added 2 commits May 17, 2022 12:27
### Summary

It is inefficient to create deep copies of tables when e.g. looping through
database rows. In my testing with uploading some 64k routes with dbless
(which calls process auto fields twice), this can cut the time looping the
data by 1/4th. It also generates much less garbage.

I searched our code bases where we use "select" context, and could not
find anything that might break because of this.
@bungle bungle force-pushed the perf/no-deep-clone-on-read branch from dba3fef to 196fb3f Compare May 17, 2022 09:27
@bungle bungle merged commit e80acd0 into master May 17, 2022
@bungle bungle deleted the perf/no-deep-clone-on-read branch May 17, 2022 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants