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

[refactor](move-memtable) remove phmap and use shared ptr in delta writer v2 #30949

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

kaijchen
Copy link
Contributor

@kaijchen kaijchen commented Feb 7, 2024

Proposed changes

  1. Remove phmap in delta writer v2 pool, also address the issue in [fix](move-memtable) fix use-after-free in DeltaWriterV2Map::get_or_create #30933.
  2. Use shared ptr of delta writer v2 in vtablet writer v2.
  3. Add ENABLE_FACTORY_CREATOR to delta writer v2.

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@doris-robot
Copy link

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

@kaijchen
Copy link
Contributor Author

kaijchen commented Feb 7, 2024

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -30,12 +30,15 @@ DeltaWriterV2Map::DeltaWriterV2Map(UniqueId load_id, int num_use, DeltaWriterV2P

DeltaWriterV2Map::~DeltaWriterV2Map() = default;

DeltaWriterV2* DeltaWriterV2Map::get_or_create(
std::shared_ptr<DeltaWriterV2> DeltaWriterV2Map::get_or_create(
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'get_or_create' can be made static [readability-convert-member-functions-to-static]

Suggested change
std::shared_ptr<DeltaWriterV2> DeltaWriterV2Map::get_or_create(
static std::shared_ptr<DeltaWriterV2> DeltaWriterV2Map::get_or_create(

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 36.16% (8635/23878)
Line Coverage: 28.19% (70605/250456)
Region Coverage: 27.18% (36436/134039)
Branch Coverage: 23.97% (18661/77852)
Coverage Report: http://coverage.selectdb-in.cc/coverage/3fa4e7ed486c84d9331069192cc7b516d1a3b1ac_3fa4e7ed486c84d9331069192cc7b516d1a3b1ac/report/index.html

@doris-robot
Copy link

TPC-H: Total hot run time: 37327 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 3fa4e7ed486c84d9331069192cc7b516d1a3b1ac, data reload: false

------ Round 1 ----------------------------------
q1	17908	4636	4596	4596
q2	2667	144	142	142
q3	11849	958	931	931
q4	4966	800	755	755
q5	8030	2971	2875	2875
q6	183	121	121	121
q7	1107	731	721	721
q8	9318	2013	1991	1991
q9	7261	6376	6344	6344
q10	8079	2467	2437	2437
q11	425	206	209	206
q12	767	294	286	286
q13	18018	3314	3300	3300
q14	302	234	243	234
q15	521	495	497	495
q16	480	400	431	400
q17	937	458	492	458
q18	6865	5953	5900	5900
q19	1566	1468	1425	1425
q20	635	335	339	335
q21	7389	3075	3070	3070
q22	800	318	305	305
Total cold run time: 110073 ms
Total hot run time: 37327 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4537	4505	4439	4439
q2	333	235	245	235
q3	2985	2782	2782	2782
q4	1857	1692	1602	1602
q5	5149	5212	5222	5212
q6	190	115	114	114
q7	2178	1707	1727	1707
q8	3104	3299	3223	3223
q9	8331	8320	8256	8256
q10	5781	3584	3574	3574
q11	545	451	464	451
q12	723	562	565	562
q13	16399	3055	3014	3014
q14	269	260	270	260
q15	538	490	485	485
q16	521	472	469	469
q17	1849	1727	1699	1699
q18	7981	7874	7362	7362
q19	7763	1498	1497	1497
q20	2157	1916	1884	1884
q21	4897	4603	4634	4603
q22	598	470	470	470
Total cold run time: 78685 ms
Total hot run time: 53900 ms

@doris-robot
Copy link

TPC-DS: Total hot run time: 174237 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 3fa4e7ed486c84d9331069192cc7b516d1a3b1ac, data reload: false

query1	924	342	338	338
query2	6537	2036	1973	1973
query3	6702	201	202	201
query4	31882	21956	21930	21930
query5	4238	435	423	423
query6	257	173	169	169
query7	4599	290	280	280
query8	235	173	191	173
query9	9038	2261	2247	2247
query10	410	210	224	210
query11	18255	15343	15251	15251
query12	134	74	75	74
query13	1617	416	411	411
query14	9501	7000	6799	6799
query15	240	182	180	180
query16	8124	270	246	246
query17	1851	533	503	503
query18	2108	270	261	261
query19	361	137	137	137
query20	81	74	78	74
query21	190	128	121	121
query22	4894	4758	4496	4496
query23	30723	30046	29933	29933
query24	11431	2753	2842	2753
query25	599	354	336	336
query26	1762	145	145	145
query27	2845	305	315	305
query28	7547	1850	1824	1824
query29	981	627	608	608
query30	283	131	140	131
query31	923	715	707	707
query32	96	58	48	48
query33	733	225	227	225
query34	1103	460	468	460
query35	852	758	768	758
query36	1013	929	932	929
query37	254	60	61	60
query38	3224	3118	3099	3099
query39	1297	1255	1240	1240
query40	272	90	92	90
query41	38	34	34	34
query42	95	89	87	87
query43	524	486	513	486
query44	1087	691	700	691
query45	197	179	176	176
query46	1043	661	658	658
query47	1548	1517	1507	1507
query48	434	386	364	364
query49	1239	288	277	277
query50	762	388	376	376
query51	5208	5103	5114	5103
query52	95	83	85	83
query53	348	264	264	264
query54	295	217	218	217
query55	78	75	76	75
query56	213	200	197	197
query57	1092	910	921	910
query58	196	179	174	174
query59	2577	2396	2253	2253
query60	246	214	211	211
query61	86	87	88	87
query62	688	339	343	339
query63	297	269	269	269
query64	6388	3744	3801	3744
query65	3252	3227	3215	3215
query66	1371	315	310	310
query67	14421	14124	14017	14017
query68	4019	544	553	544
query69	473	323	330	323
query70	1253	1211	1261	1211
query71	303	248	250	248
query72	5911	2836	2691	2691
query73	696	332	325	325
query74	6452	6205	6300	6205
query75	3021	2378	2334	2334
query76	2502	1009	954	954
query77	322	227	226	226
query78	9118	8817	8473	8473
query79	3504	492	502	492
query80	2051	349	345	345
query81	528	194	193	193
query82	867	88	85	85
query83	246	123	132	123
query84	281	79	78	78
query85	2382	339	322	322
query86	498	306	294	294
query87	3402	3164	3184	3164
query88	4589	2381	2405	2381
query89	472	373	365	365
query90	2075	165	166	165
query91	153	125	122	122
query92	56	45	42	42
query93	5213	519	483	483
query94	1302	174	174	174
query95	8074	7829	357	357
query96	600	276	283	276
query97	4246	4125	4105	4105
query98	223	206	197	197
query99	1108	723	668	668
Total cold run time: 297874 ms
Total hot run time: 174237 ms

@doris-robot
Copy link

ClickBench: Total hot run time: 30.61 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 3fa4e7ed486c84d9331069192cc7b516d1a3b1ac, data reload: false

query1	0.03	0.03	0.03
query2	0.06	0.03	0.02
query3	0.22	0.06	0.06
query4	1.68	0.10	0.11
query5	0.54	0.52	0.53
query6	1.22	0.63	0.64
query7	0.01	0.01	0.02
query8	0.03	0.02	0.03
query9	0.55	0.49	0.50
query10	0.56	0.57	0.56
query11	0.12	0.09	0.08
query12	0.11	0.09	0.10
query13	0.59	0.60	0.61
query14	0.79	0.82	0.80
query15	0.78	0.78	0.78
query16	0.38	0.38	0.37
query17	1.04	1.05	1.02
query18	0.22	0.28	0.23
query19	1.89	1.77	1.80
query20	0.01	0.01	0.02
query21	15.42	0.57	0.54
query22	2.70	2.99	1.74
query23	16.99	1.00	0.80
query24	2.57	1.10	0.68
query25	0.33	0.27	0.10
query26	0.42	0.15	0.14
query27	0.06	0.05	0.06
query28	11.93	0.83	0.87
query29	12.50	3.18	3.27
query30	0.58	0.55	0.46
query31	2.80	0.35	0.35
query32	3.34	0.49	0.48
query33	3.21	3.23	3.23
query34	15.89	4.29	4.30
query35	4.35	4.35	4.25
query36	1.09	1.06	1.05
query37	0.06	0.05	0.05
query38	0.03	0.02	0.02
query39	0.02	0.01	0.02
query40	0.16	0.13	0.13
query41	0.06	0.01	0.02
query42	0.02	0.01	0.01
query43	0.02	0.02	0.02
Total cold run time: 105.38 s
Total hot run time: 30.61 s

@doris-robot
Copy link

Load test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G'

Load test result on commit 3fa4e7ed486c84d9331069192cc7b516d1a3b1ac with default session variables
Stream load json:         19 seconds loaded 2358488459 Bytes, about 118 MB/s
Stream load orc:          59 seconds loaded 1101869774 Bytes, about 17 MB/s
Stream load parquet:      31 seconds loaded 861443392 Bytes, about 26 MB/s
Insert into select:       13.5 seconds inserted 10000000 Rows, about 740K ops/s

Copy link
Contributor

@liaoxin01 liaoxin01 left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Feb 7, 2024
Copy link
Contributor

github-actions bot commented Feb 7, 2024

PR approved by at least one committer and no changes requested.

Copy link
Contributor

github-actions bot commented Feb 7, 2024

PR approved by anyone and no changes requested.

@yiguolei yiguolei merged commit 960e80c into apache:master Feb 7, 2024
28 of 30 checks passed
yiguolei pushed a commit that referenced this pull request Feb 8, 2024
…iter v2 (#30949)

* [refactor](move-memtable) remove phmap and use shared ptr in delta writer v2 pool

* ENABLE_FACTORY_CREATOR DeltaWriterV2
yiguolei pushed a commit that referenced this pull request Feb 16, 2024
…iter v2 (#30949)

* [refactor](move-memtable) remove phmap and use shared ptr in delta writer v2 pool

* ENABLE_FACTORY_CREATOR DeltaWriterV2
mymeiyi pushed a commit to mymeiyi/doris that referenced this pull request Feb 19, 2024
…iter v2 (apache#30949)

* [refactor](move-memtable) remove phmap and use shared ptr in delta writer v2 pool

* ENABLE_FACTORY_CREATOR DeltaWriterV2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by one committer. reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants