[CBRD-27244] Improve string block search for large statements - #7715
Open
youngjinj wants to merge 24 commits into
Open
[CBRD-27244] Improve string block search for large statements#7715youngjinj wants to merge 24 commits into
youngjinj wants to merge 24 commits into
Conversation
The arena's string blocks lived in a global 128-bucket hash keyed by parser id, so every allocation, append and free scanned other parsers' blocks under a global mutex, and a long statement paid O(blocks) per string just in id compares. The block list now lives in PARSER_CONTEXT itself: only the owner scans it, newest block first, with no lock. On an 8-table sysbench prepare (5M rows, 2,688 rows per statement) cub_cas CPU drops from 397.5s to 379.6s on top of the parser fixes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
parser_allocate_string_buffer walked the whole block list looking for room. A long statement builds hundreds of blocks, and a block leaves the head exactly when a request does not fit it, so the room behind the head is small and the walk almost always ended empty-handed: the scan was 85% of this function's own time. It now looks at the newest few blocks (STRING_BLOCK_SCAN_LIMIT) and takes a new block otherwise. The depth was picked by sweeping it (ten runs each) against a sequence built to strand a reusable tail behind the head: peak RSS falls as the depth grows and stops falling once it reaches the current value, matching the full walk, while wall time does not respond to the depth at all -- only removing the bound does, and that costs three times the time. So the depth buys the reuse the walk existed for at no measured cost. On a statement whose printing is dominated by constant folding, wall time drops from 10.7s to 3.6s; on an 8-table sysbench prepare cub_cas CPU drops from 348.0s to 329.3s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
❌ TC Merge Gate — Merge BlockedOne or more TC PRs are still open. Please merge or close them before merging this PR. TC Repositories & Branches:
Steps to unblock:
|
🧪 TC Test Environment ReadyCircleCI Testing:
TC Repositories & Branches:
Next Steps:
|
The id was the ownership tag for the global hash bucket: the allocation scan and the selective free used it to skip other parsers' blocks. With the block list owned by its parser both readers are gone, and the two remaining lookups (in-place-append check, single-block free) match by pointer. The field was write-only; removing it does not shrink the struct (alignment), it just stops implying an ownership check that no longer exists. Also condense the scan-limit and string_blocks field comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pt_find_block_with_room now answers the allocation scan: it checks each of the newest blocks with the same DB_ALIGN placement the caller then performs, so a passing candidate is guaranteed to fit, where the old worst-case test (length + align - 1 + 1) could skip a block that had room. The worst case survives only as the size of a fresh block. parser_create_string_block's two branches differed only in size and block_end, so they now share one malloc and one failure path, and the 1001 bytes an oversized block keeps past its one large string gets a name, LARGE_STRING_APPEND_ROOM. PARSER_STRING_BLOCK moves to the header's forward-declaration cluster, typing the parser's string_blocks field and dropping the casts at every use site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The whole family took const PARSER_CONTEXT * and shed it with a cast wherever it touched parser->string_blocks, so every signature claimed not to modify the parser while doing exactly that. The mutators -- create, free-one, free-all, the two append workers and the public allocator (which has no caller outside this file) -- now take the parser non-const; the read-only lookups keep const, which finally means something. The public append API keeps its historical const contract (17 files use it), so the cast survives only at that boundary, one per entry point, as pt_append_bytes already did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
STRINGS_PER_BLOCK becomes STRING_BLOCK_DEFAULT_SIZE -- it is the byte size of a default block's string area, not a count of strings -- and the header-plus-malloc overhead it subtracts moves into its own STRING_BLOCK_OVERHEAD constant. BLOCK_LAST_STRING and BLOCK_ROOM_LEFT, defined right under the struct whose fields they read, replace the spelled-out forms of the block's last-string pointer (five sites) and of the room test the in-place appends share (two sites). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
STRING_BLOCK_OVERHEAD hand-estimates the header and malloc bookkeeping a block must leave room for. The static_assert fails the build if the struct outgrows that estimate, instead of letting the allocation silently spill into the next size class. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BLOCK_LAST_STRING and BLOCK_ROOM_LEFT become PT_STRBLK_LAST_START and PT_STRBLK_AVAILABLE_SIZE: the parser-module prefix marks whose block they read, LAST_START matches the last_string_start field behind it, and AVAILABLE_SIZE names the byte count still open past the last string. LAST_START yields the character itself rather than its address, so a call site wanting the pointer takes it explicitly, the way &buf[i] reads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The hand-estimated (4*sizeof(long)+sizeof(char *)+40) becomes the aligned header fields in front of the string area, so a default block struct is exactly 8KB. The field list mirrors the struct, and the static_assert below the struct fails the build the moment the two drift apart. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The helper answers "which string block still has enough available bytes", so its name now lines up with pt_find_string_block and uses the same term as PT_STRBLK_AVAILABLE instead of the colloquial "with room". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A fresh block places its first string at offset 0; the call reads that way without the note. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
"one" states the unit more plainly than the article and pairs with pt_free_string_blocks, which frees the whole list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The helper looks a block up by its key: which block holds old_string as its last string. The name now says so, and the walker variable is a block, not a string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
parser.h keeps const on parser_alloc and the pt_append family for their many callers, so each entry point casts it away before filling this parser's own string blocks. The casts now say why, and parser_create_string_block drops a cast left from its const days. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The definition sat ahead of parser_create_string_block while the declarations put it after pt_find_block_by_last_string. The two orders now match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The constant is the byte count an oversized block reserves past its one large string, so "reserve" states the role where "room" was colloquial. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The one-line form hid why STRING_BLOCK_DEFAULT_SIZE was subtracted from sizeof: the difference is just the block's header, which STRING_BLOCK_OVERHEAD already names -- the static_assert pins the two as equal. The allocation now reads as the overhead plus the string area this block wants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The renamed pt_find_available_string_block declaration passed 120 columns, and pt_append_bytes_for's wrap point moved when it lost its const. Both now match what indent -l120 -lc120 produces, keeping the style check clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
/run all |
1 similar comment
Contributor
Author
|
/run all |
youngjinj
marked this pull request as ready for review
August 24, 2026 09:49
Contributor
Author
|
@greptile 리뷰해줘 |
Contributor
|
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'upstream/d..." | Re-trigger Greptile |
The field sat between id and statement_number, where the old global array's parser_id % 128 keying had put it. That keying is gone, so the list now joins orphans -- the other allocation this parser owns and frees when it dies -- and the two int fields are adjacent again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
/run all |
| * keeps refusing the similar ones that follow. The newest few still catch a smaller request | ||
| * fitting a recent block's tail. | ||
| */ | ||
| #define STRING_BLOCK_SCAN_LIMIT 8 |
Contributor
There was a problem hiding this comment.
방향과 산술 모두 확인했습니다 — 전역 배열이 공유 목적이 아니었다는 판단과 뮤텍스 제거 정당성에 동의하고, 오버사이즈 블록 산식이 구식과 비트 동일한 것, length == STRING_BLOCK_DEFAULT_SIZE - 1 경계가 기본 블록에 정확히 안착하는 것까지 대조했습니다.
비차단 제안 하나: 스캔 상한 8의 단편화 최악 케이스를 Remarks 에 한 줄 남겨 주시면 좋겠습니다. 8칸 창을 벗어난 블록의 잔여 공간은 파서 수명 동안 죽은 공간이 되고, 큰/작은 문자열이 교차하는 워크로드에서 낭비 상한은 (미스된 블록 수 × ~8KB)입니다. 파서 수명이 짧고 sysbench 에서 cas 메모리 불변을 확인하셨으니 실질 무해하다고 보는데, "왜 8이고 최악 케이스가 왜 무해한가"가 본문에 있으면 나중에 이 상수를 재조정할 사람이 근거를 다시 추적하지 않아도 됩니다.
Contributor
Author
There was a problem hiding this comment.
상한값 실측(1/4/8/16/무제한)은 JIRA 에 첨부한 설계 문서(cbrd-27244_design_*.md) 4절 "상한 값의 근거" 에 있었고, 말씀하신 최악의 경우는 이번 의견을 받아 같은 4절에 "상한이 만드는 최악의 경우" 로 추가했습니다. 첨부도 갱신본으로 교체했습니다. 실측은 sysbench prepare(8테이블 x 500만 행) 시나리오로, 상한만 바꿔 빌드해 각 1회 실행한 것입니다.
추가한 내용은 이렇습니다. 최악의 경우는 작지 않은 빈 공간이 남은 PARSER_STRING_BLOCK 이 탐색 범위 밖으로 밀려나 그 문장이 끝날 때까지 쓰이지 않는 상태입니다. 블록은 만들어질 때마다 목록 앞에 붙으므로 밀어내는 것은 새 블록 생성이고, 두 갈래가 있습니다.
- STRING_BLOCK_DEFAULT_SIZE 를 넘는 문자열은 전용 블록(length + LARGE_STRING_APPEND_RESERVE)을 받습니다. 재사용 여지가 거의 없으면서 탐색 범위 한 칸을 차지하므로, 그런 블록이 앞의 8칸을 채우면 그 뒤 블록의 빈 공간은 찾지 못합니다. 다만 8개 블록이 모두 그 크기를 넘어야 해서 자주 생기는 상황은 아닙니다.
- 남은 빈 공간보다 큰 요청과 짧은 요청이 번갈아 오면 큰 요청이 매번 새 블록을 만들고, 그때마다 탐색 범위가 한 칸씩 밀립니다. 이쪽은 상대적으로 자주 생길 수 있지만 낭비되는 빈 공간이 크다고 단정할 수는 없습니다.
무해하다고 단정하지는 않습니다. 다만 낭비는 (밀려난 블록 수) x (블록당 잔여, 8KB 미만) 이고 세 가지로 제한됩니다 -- 파서가 소멸할 때 목록 전체가 반환되어 영구적이지 않고, 목록이 파서 소유라 다른 파서로 번지지 않으며, 실측에서 상한 8 과 무제한의 cas 최대 RSS 가 같았습니다. 파싱된 문자열의 수명이 짧다는 점에서 빈 공간을 찾는 비효율이 낭비보다 크다고 보고 8 을 절충점으로 골랐습니다.
| static_assert (sizeof (PARSER_STRING_BLOCK) == 8192, "a default string block must be exactly 8KB"); | ||
|
|
||
| /* the first character of the last string placed in the block */ | ||
| #define PT_STRBLK_LAST_START(b) ((b)->u.chars[(b)->last_string_start]) |
Contributor
There was a problem hiding this comment.
스타일 제안 (비차단): PT_STRBLK_LAST_START(b) 는 평가 결과가 char lvalue 인데 이름이 위치처럼 읽혀서, 호출부의 &PT_STRBLK_LAST_START (block) 와 PT_STRBLK_LAST_START (block) = 0; 이 이중으로 꼬여 보입니다. 포인터를 돌려주는 형태 — 예: PT_STRBLK_LAST_STRING(b) = (&(b)->u.chars[(b)->last_string_start]) — 로 하면 세 호출부가 *PT_STRBLK_LAST_STRING (block) = 0; / return PT_STRBLK_LAST_STRING (block); 으로 읽기 쉬워집니다.
Contributor
Author
There was a problem hiding this comment.
반영했습니다. PT_STRBLK_LAST_START 를 포인터를 돌려주는 PT_STRBLK_LAST_STRING 으로 바꾸고 호출부 5곳을 함께 고쳤습니다. 4곳에서 & 가 사라지고 널 종결 한 곳만 PT_STRBLK_LAST_STRING(block) = 0; 이 됩니다.
Four of the five uses took the address of PT_STRBLK_LAST_START, and every one of them fed a char pointer, so the macro now yields the pointer itself and only the nul termination dereferences it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/run all |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
http://jira.cubrid.org/browse/CBRD-27244
Purpose
파서 문자열 저장소가 자리 남은 블록을 찾을 때 전역 배열에 매달린 다른 파서의 블록까지 목록 끝까지 훑는다. 블록 목록을 파서마다 나누고 확인하는 블록 수에 상한을 두어, 남의 블록을 훑는 일과 훑는 길이가 늘어나는 일을 함께 없앤다.
Implementation
parser_String_blocks[]를 없애고 각PARSER_CONTEXT가 자기 블록 목록을 갖는다. 담기/찾기/반납이 자기 목록만 순회한다.SERVER_MODE빌드에만 있던 전역 잠금 획득/해제가 함께 사라진다.parser_allocate_string_buffer가 최신 블록부터STRING_BLOCK_SCAN_LIMIT(8) 개까지만 보고, 없으면 새 블록을 만든다.Remarks
parser_id를 들고 있고 순회가block->parser_id != parser->id로 남의 블록을 걸러내기만 한다 -- 목록을 파서 문맥에 두면 그 필터가 항상 참이 되어 사라진다.parse_tree.c를 포함하지 않고, 이 파일이 들어가는 cs/sa 타깃은SERVER_MODE가 아니다).배경 근거와 실측 상세는 JIRA 의 설계/테스트 문서 참고.