docs: address document-review findings for resreq-ast-cse-design
Multi-persona review (coherence, feasibility, adversarial, product-lens) identified 25 findings. Applied 14 fixes: - Fix makeSelectNode key_add return code bug (rc2 → rc3) - Resolve derefSelectNode contradiction: remove function and refcount field, adopt GC-only reclamation model - Add boolean_literal grammar rule (true/false keywords) and SEL_BOOL_LITERAL node type + bval union member - Clarify transform pipeline: makeSelectNode does commutativity swap internally; flattening/const-fold are post-parse tree walks - Complete const-fold rules with annihilation laws - Add SEL_NUM bit-cast semantics (±0 distinction, NaN payloads) - Add resmgr=NULL fallback behavior spec - Add runtime type safety assertions for debug builds - Add GC trigger and session-based scheduler context - Fix section numbering gap (renumber §5.6 → §5.5) - Add gc_sweep_select_nodes/gc_mark_tree to Phase 2 declarations - Update kill_primary_resreq instruction for GC model - Add any keyword semantic inconsistency note (!= vs !(==)) 4 open issues remain for implementation phase: string resource interface, single-goroutine enforcement, kvtab iteration API, CSE benefit quantification.
This commit is contained in:
parent
33b76d9a77
commit
877741d008
1 changed files with 94 additions and 54 deletions
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue