diff --git a/src/docs/resreq-ast-cse-design.md b/src/docs/resreq-ast-cse-design.md index cf1fd653..1339313e 100644 --- a/src/docs/resreq-ast-cse-design.md +++ b/src/docs/resreq-ast-cse-design.md @@ -101,10 +101,11 @@ atom_expr ::= '(' or_expr ')' | comparison | resname comparison ::= resname cmp_op value cmp_op ::= '>=' | '<=' | '!=' | '==' | '>' | '<' resname ::= [a-zA-Z_][a-zA-Z_0-9]* -value ::= string_literal | number | ANY +value ::= string_literal | number | boolean_literal | ANY +boolean_literal ::= 'true' | 'false' ANY ::= 'any' number ::= [0-9]+ ('.' [0-9]*)? -string_literal ::= [a-zA-Z_][a-zA-Z_0-9]* (unquoted word, excluding 'any') +string_literal ::= [a-zA-Z_][a-zA-Z_0-9]* (unquoted word, excluding 'any', 'true', 'false') ``` **LSF-compatible disambiguation:** `resname` and `string_literal` share @@ -121,6 +122,23 @@ only valid as the right-hand side of `==` or `!=` (e.g., `type == any`, The const-fold pass (§6.3) reduces `resname == any` to `SEL_TRUE`, so `eval_select` never encounters `SEL_ANY` at runtime. +**Note on `!= any`:** `resname != any` is a parse error. However, +`!(resname == any)` const-folds to `!SEL_TRUE` which reduces to +`SEL_FALSE` — these are semantically equivalent. This inconsistency +exists because the parse-time rejection of `!= any` provides a clearer +error message for users, while the const-fold path handles the negated +form implicitly. If this inconsistency proves confusing in practice, +a future change can allow `resname != any` at parse time and const-fold +it to `SEL_FALSE`. + +**Boolean literals:** `true` and `false` are parsed as `SEL_BOOL_LITERAL` +nodes. The value is stored as `int bval` (1 for true, 0 for false) in +the node's union. Type validation ensures that boolean literals only +appear in `==`/`!=` comparisons with `RES_TYPE_BOOL` resources, or as +standalone boolean operands (where they const-fold to `SEL_TRUE` or +`SEL_FALSE`). `true` and `false` are reserved keywords excluded from +the `string_literal` pattern. + ### 3.2 Node Types ```c @@ -137,6 +155,7 @@ typedef enum { SEL_RESNAME, /* leaf: resource variable */ SEL_NUM, /* leaf: numeric literal */ SEL_STR, /* leaf: string literal */ + SEL_BOOL_LITERAL, /* leaf: boolean literal (true/false) */ SEL_ANY, /* leaf: any keyword (const-folded to SEL_TRUE) */ SEL_TRUE, /* leaf: constant true (from const fold) */ SEL_FALSE, /* leaf: constant false (from const fold) */ @@ -148,7 +167,6 @@ typedef enum { ```c struct xce_select_node_ { sel_node_type_t type; - atomic_ulong refcount; /* intern table reference count */ uint32_t hashcode; /* cached at construction */ unsigned gc_mark; /* set during mark-and-sweep GC (§5.6) */ union { @@ -162,6 +180,7 @@ struct xce_select_node_ { } resname; double numval; /* SEL_NUM */ char* strval; /* SEL_STR — interned via makeString() */ + int bval; /* SEL_BOOL_LITERAL — 1 for true, 0 for false */ } u; }; ``` @@ -279,10 +298,17 @@ Key encoding per node type: | `SEL_RESNAME` | `(uintptr_t)res_idx` | `0` | | `SEL_NUM` | bit-cast of `double` | `0` | | `SEL_STR` | `(uintptr_t)xstring_ptr` | `0` | +| `SEL_BOOL_LITERAL` | `(uintptr_t)bval` | `0` | | `SEL_ANY` | `0` | `0` | | `SEL_TRUE` | `0` | `0` | | `SEL_FALSE` | `0` | `0` | +`SEL_NUM` uses the exact bit-pattern of the `double` value reinterpreted as +`uintptr_t` (via `memcpy` or union cast). Two numeric literals are +equivalent for CSE if their bit-patterns are identical; `0.0` and `-0.0` +are distinct CSE keys (they differ in the sign bit). NaN payloads are +also compared by bit-pattern. + `SEL_STR` uses the interned `xstring` pointer — since `makeString()` already canonicalizes strings (libxce's string de-dup mechanism maps identical strings to the same pointer), pointer equality IS string equality. @@ -322,9 +348,8 @@ xce_select_node_t* makeSelectNode(sel_node_type_t type, xce_select_node_t* node = NULL; mylock_lock(&sel_node_table.lock); int rc = key_find(sel_node_table.table, &key, (void**)&node); - if (rc > 0) reference(node); + if (rc > 0) { mylock_unlock(&sel_node_table.lock); return node; } mylock_unlock(&sel_node_table.lock); - if (rc > 0) return node; /* cache hit */ if (rc < 0) return NULL; /* key_find error (not ENOENT) */ /* 4. alloc and fill */ @@ -332,7 +357,6 @@ xce_select_node_t* makeSelectNode(sel_node_type_t type, if (node == NULL) { errno = ENOMEM; return NULL; } node->type = type; node->hashcode = sel_key_hash(&key); - atomic_store(&node->refcount, 1); /* fill u.* from op1/op2 by type … */ /* 5. insert — re-check under lock for TOCTOU safety */ @@ -341,14 +365,13 @@ xce_select_node_t* makeSelectNode(sel_node_type_t type, int rc2 = key_find(sel_node_table.table, &key, (void**)&existing); if (rc2 > 0) { /* another thread inserted first — use theirs, free ours */ - reference(existing); mylock_unlock(&sel_node_table.lock); free(node); return existing; } - key_add(sel_node_table.table, &key, node); - if (rc2 < 0) { - /* key_add failed (EEXIST check already passed, likely ENOMEM) */ + int rc3 = key_add(sel_node_table.table, &key, node); + if (rc3 < 0) { + /* key_add failed (likely ENOMEM or EEXIST from a race) */ mylock_unlock(&sel_node_table.lock); free(node); return NULL; @@ -365,28 +388,18 @@ select intern table adds the re-check to prevent node leaks under concurrent construction. An alternative is to hold the lock through the entire sequence (simpler but longer critical section). -```c -void derefSelectNode(xce_select_node_t* node) -{ - if (node == NULL) return; - atomic_fetch_sub(&node->refcount, 1); - /* Reclamation is handled by mark-and-sweep GC (§5.6). - When a job completes, kill_primary_resreq calls derefSelectNode - on the root to decrement refcounts. The GC sweep between - sessions frees nodes that are no longer reachable from any - live job's resreq root. */ -} -``` +`kill_primary_resreq()` in `resreq.c` must unlink the select root +pointer (`primary->select->root = NULL`) before freeing the select struct. +Node reclamation is handled by mark-and-sweep GC (§5.5) — no explicit +deref function is needed. -`kill_primary_resreq()` in `resreq.c` must call -`derefSelectNode(primary->select->root)` before freeing the select struct. +### 5.5 Mark-and-Sweep GC Between Scheduling Sessions -### 5.6 Mark-and-Sweep GC Between Scheduling Sessions - -`derefSelectNode` only decrements refcounts — it does **not** free nodes -or remove them from the intern table. Reclamation is handled by a -**mark-and-sweep** GC that runs between scheduling sessions, when no -evaluation is in progress: +Node reclamation is handled by a **mark-and-sweep** GC that runs +between scheduling sessions, when no evaluation is in progress. There +is no `derefSelectNode` function — when a job completes, its resreq +entry is simply unlinked from the job (see §5.4); the next GC sweep +reclaims unreachable nodes. ``` gc_sweep_select_nodes() @@ -409,14 +422,22 @@ This approach is simple and correct: - A node is reachable if and only if it is visited from at least one live job's resreq root. No reference provenance tracking is needed — the mark flag is a single boolean per node. -- `derefSelectNode` is removed — when a job completes, its resreq entry - is simply unlinked from the job; the next GC sweep reclaims unreachable - nodes. - The sweep runs only between sessions (no concurrent eval), so no read-after-free risk exists. - `SEL_STR` nodes' `xstring` references are handled by the same mark pass — if the SEL_STR node is reachable, the string stays interned. +The scheduler is responsible for calling `gc_sweep_select_nodes()` +between scheduling sessions, typically after all dispatch/defer decisions +for the current batch of jobs have been made. The function is not +thread-safe with respect to concurrent `eval_select` calls, so the +scheduler must ensure no evaluations are in progress when GC runs. + +Like IBM LSF, the XCE scheduler operates in sessions — each session +evaluates all pending jobs against candidate hosts, dispatches what it +can, then ends. Pre/post-session work (such as GC) is a natural fit +for this architecture. + **Pointer-based canonicalization note:** Sorting by `uintptr_t` produces a deterministic total order within a single process lifetime. Across process restarts, pointer addresses change (ASLR, heap layout), so the @@ -472,9 +493,9 @@ are statically determined at parse time: resource value). The `any` keyword (§3.1) is only valid on the right-hand side of `==` or `!=`. For `!=`, `resname != any` is a parse error (nothing would not match any). -- `SEL_TRUE && X` → `X`, `X && SEL_TRUE` → `X` (identity elimination) -- `SEL_FALSE || X` → `X`, `X || SEL_FALSE` → `X` (identity elimination) -- `!SEL_TRUE` → `SEL_FALSE`, `!SEL_FALSE` → `SEL_TRUE` +- Identity laws:`SEL_TRUE && X` → `X`, `X && SEL_TRUE` → `X`; `SEL_FALSE || X` → `X`, `X || SEL_FALSE` → `X` +- Annihilation laws: `SEL_FALSE && X` → `SEL_FALSE`, `X && SEL_FALSE` → `SEL_FALSE`; `SEL_TRUE || X` → `SEL_TRUE`, `X || SEL_TRUE` → `SEL_TRUE` +- Complement laws: `!SEL_TRUE` → `SEL_FALSE`, `!SEL_FALSE` → `SEL_TRUE` The const-fold pass runs **after** construction and flattening, replacing subtrees with constant leaf nodes. Since all nodes are interned, @@ -491,17 +512,23 @@ AST construction follows a fixed pipeline after the PEG parser produces the raw tree: ``` -1. Parse (PEG → raw binary AST) -2. Commutativity canonicalization (operand swap, §6) -3. Associativity flattening (§6.2) -4. Const-fold (§6.3) -5. CSE intern (makeSelectNode for each node bottom-up) +1. Parse (PEG → raw binary AST, each PEG action calls `makeSelectNode` + which performs commutativity canonicalization as part of CSE key + construction) +2. Post-parse tree walk: associativity flattening (§6.2) +3. Post-parse tree walk: const-fold (§6.3) +4. Bottom-up re-intern via `makeSelectNode` for any modified subtrees ``` -Steps 2–4 operate on the tree before interning (they construct new -nodes via `makeSelectNode` which itself does the commutativity swap). -Step 5 re-interns any modified subtrees, ensuring CSE maximizes sharing -on the fully normalized tree. +Note: commutativity canonicalization (operand swap) happens *inside* +`makeSelectNode` as part of the CSE key lookup — it is not a separate +pipeline step. The PEG action for each binary operator calls +`makeSelectNode`, which sorts `op1`/`op2` for commutative operators +before building the key. Flattening and const-fold are post-parse tree +walks that traverse the raw AST and call `makeSelectNode` to construct +new normalized subtrees. Since `makeSelectNode` already handles interning +and canonicalization, these walks naturally produce the maximally shared +tree. --- @@ -578,6 +605,14 @@ See §12 for the formal declaration. Unknown resource names fail the parse (rejected at job submission time). +**resmgr=NULL fallback:** When `resmgr` is NULL (as called by the +existing `parse_resreq_primary` entry point), the parser cannot +disambiguate resname from string_literal. In this mode, all bare +words are treated as `SEL_STR` and no type validation is performed — +the caller must validate type compatibility before evaluation. The +new entry point `parse_resreq_primary_with_resmgr` should be preferred +whenever the resource manager is available. + The **resmgr** (resource manager) is simply an array of resource definitions — each entry maps a resource name to its type and other metadata. The API server has access to this information via the cluster @@ -712,6 +747,11 @@ int eval_select(const xce_select_node_t* node, const xvec_idval_t* host_resvals) both operands of EQ/NE are type-compatible: either both numeric, both string, or a RESNAME paired with a literal whose type matches the resource's declared type. See §7 for type coercion rules. +- **Runtime type safety:** In debug builds, `eval_select` should include + `assert()` checks on operand types for `SEL_GT`/`SEL_LT`/`SEL_GE`/`SEL_LE` + branches to verify both operands are `RES_TYPE_NUMERIC`. In production + builds, these branches return `-1` on type mismatch as a defensive fallback, + since parse-time validation should prevent such cases from reaching eval. ### 8.4 Thread Safety @@ -803,8 +843,8 @@ Key properties: - Parse: valid expressions, unknown resource names (expect error), type mismatches (numeric op on string resource, expect error). - CSE: `A && B` and `B && A` produce the same `xce_select_node_t*`. - - CSE: two calls to `makeSelectNode` with same args return same pointer, - refcount increases. + - CSE: two calls to `makeSelectNode` with same args return same pointer + (same interned node). - Eval: numeric `>`, `<`, `==`; string `==`; bool standalone; `!`, `&&`, `||`; short-circuit evaluation. @@ -818,8 +858,9 @@ signatures and assertions are defined before implementation code.* - Add `res_type_t`, `sel_node_type_t`, `xce_select_node_t` typedef + struct. - Fill `struct xce_resreq_select_` with `xce_select_node_t* root`. - - Declare `makeSelectNode`, `derefSelectNode`, `eval_select`, - `sel_node_table_init`, `sel_node_table_fini`. + - Declare `makeSelectNode`, `eval_select`, + `sel_node_table_init`, `sel_node_table_fini`, `gc_sweep_select_nodes`, + `gc_mark_tree`. - Add `res_type_to_valtype` / `valtype_to_res_type` mapping helpers. - Add `parse_resreq_primary_with_resmgr(primary, resreqString, errmsg, resmgr)` as a new entry point. The existing `parse_resreq_primary` calls it @@ -843,7 +884,6 @@ signatures and assertions are defined before implementation code.* - `sel_node_table_init(int nslots)` / `sel_node_table_fini()`. - `makeSelectNode` (§5.4) — with key_add return code check for TOCTOU safety. - - `derefSelectNode` (§5.5). - `sel_key_of`, `sel_key_hash` helpers. - `eval_select` (§8.3) — explicit error propagation (no C boolean ops). - `host_resval_typed`, `host_get_bool`, `host_get_float`, @@ -851,7 +891,8 @@ signatures and assertions are defined before implementation code.* - Mark-and-sweep GC: `gc_sweep_select_nodes()`, `gc_mark_tree()`. - AST transform pipeline: flatten, const-fold (§6.2–6.4). - `xce_str_match` wrapper (initially `strcmp` semantics). - - Update `kill_primary_resreq` to call `derefSelectNode`. + - Update `kill_primary_resreq` to unlink select root (`primary->select->root = NULL`) + before freeing the select struct. GC between sessions reclaims unreachable nodes. - Update `parse_resreq_primary` to call `parse_select` with resmgr. #### Phase 5: Build wiring @@ -887,16 +928,15 @@ typedef enum { RES_TYPE_NUMERIC = 0, RES_TYPE_STRING, RES_TYPE_BOOL } res_type_t typedef enum { SEL_AND, SEL_OR, SEL_NOT, SEL_GT, SEL_LT, SEL_GE, SEL_LE, SEL_EQ, SEL_NE, - SEL_RESNAME, SEL_NUM, SEL_STR, SEL_ANY, SEL_TRUE, SEL_FALSE, + SEL_RESNAME, SEL_NUM, SEL_STR, SEL_BOOL_LITERAL, SEL_ANY, SEL_TRUE, SEL_FALSE, } sel_node_type_t; typedef struct xce_select_node_ xce_select_node_t; struct xce_select_node_ { sel_node_type_t type; - atomic_ulong refcount; uint32_t hashcode; - unsigned gc_mark; /* set during mark-and-sweep GC (§5.6) */ + unsigned gc_mark; /* set during mark-and-sweep GC (§5.5) */ union { struct { xce_select_node_t *left, *right; } binop; xce_select_node_t* operand;