-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
opt: 20.2 release checklist #46599
Comments
I guess we should leave "check bug fixes that may need backport" open for now since we haven't cut the branch yet. This will become relevant if we find any bugs after the branch is cut. I'm working on "check and update private opt tests repository" now |
I ran the microbenchmarks, comparing a commit from the beginning of the release with master. Here are the results:
Might be worth looking into some of the |
Thanks a lot @rytaft! Looked at some profiles for kv-read-const; they suggest the CC @mgartner I think simply allocating that map is non-trivial for this very simple query. We should allocate it lazily. I don't think this explains all the delta though (commenting out that map initialization got from 200ns to 150ns on my machine; 20.1.5 shows 100ns). |
Oh, the other difference is the join order builder, we should embed it in the Optimizer struct instead of allocating it every time. |
I've made the making of the contraint cache map in Implicator lazy: #54159 Though it made some of the microbenchmarks slower by up to 17%. I'll rerun the benchmarks tomorrow, maybe my computer decided to do some funny business during part of the benchmark. Tomorrow I can embed the join order builder in the Optimizer. |
Reran those benchmarks for the PR linked above and they look a bit better now. |
I reran the benchmarks to compare 20.1.5 (old) to #54159 (new) on a GCE worker.
|
I looked at kv-read-no-prep some more. A big part of the remaining overhead is that now we have to construct a Project, I'm assuming because of the system columns. Not sure what can be done about that.. |
Same cause for Chain - here we are doing an N-way join and each instance of the table now needs a Project. |
I also looked at I think there is a possible improvement here though - when we create a basic Scan for all the columns (which appens during optbuild, even when we don't need all columns), we don't need to make a copy of the table FDs (since we don't need to modify them). In any case, if we would analyze the AST beforehand and know when we will never need any system columns, we could do a lot better. I believe that doing a traversal and looking for system column names in all column references should work. |
Tried to skip the system columns when creating table FDs, there is a small benefit to tpcc-delivery but not for others. Anotger idea would be to move the FDs in the catalog, where they can be calculated once. We'd need a CopyFrom function that shifts all column IDs. This seems like it would save about 3-4%. I'm not thrilled by some of these, 15% in the end-to-end time for kv-read is not nothing.. |
I looked into this more closely. I see about 5% regression on my system (with and without GOMAXPROCS=1). There are about 10% more allocations; half of them appear to come from resolving table descriptors. I did not see any noticeable differences in the opt part. |
@RaduBerinde it was both commits, unless I did something wrong. I can re-run to be sure. |
Here are the results of
|
I checked backboard and did not find any missing backports. Closing this now. |
This issue captures a series of tests and steps that we need to run when 20.2 is getting close to being finalized.
TAGS=fast_int_set_small
andTAGS=fast_int_set_large
The text was updated successfully, but these errors were encountered: