Skip to content

Delete the val that never used - #553

Closed
WangTaoTheTonic wants to merge 1 commit into
apache:masterfrom
WangTaoTheTonic:master
Closed

WangTaoTheTonic wants to merge 1 commit into
apache:masterfrom
WangTaoTheTonic:master

Conversation

@WangTaoTheTonic

Copy link
Copy Markdown
Contributor

It seems that the val "startTime" and "endTime" is never used, so delete them.

@AmplabJenkins

Copy link
Copy Markdown

Can one of the admins verify this patch?

@rxin

rxin commented Apr 25, 2014

Copy link
Copy Markdown
Contributor

Jenkins, test this please.

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished. All automated tests passed.

@AmplabJenkins

Copy link
Copy Markdown

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14486/

@rxin

rxin commented Apr 25, 2014

Copy link
Copy Markdown
Contributor

Thanks. I've merged this.

asfgit pushed a commit that referenced this pull request Apr 25, 2014
It seems that the val "startTime" and "endTime" is never used, so delete them.

Author: WangTao <[email protected]>

Closes #553 from WangTaoTheTonic/master and squashes the following commits:

4fcb639 [WangTao] Delete the val that never used

(cherry picked from commit 25a276d)
Signed-off-by: Reynold Xin <[email protected]>
@asfgit asfgit closed this in 25a276d Apr 25, 2014
pwendell pushed a commit to pwendell/spark that referenced this pull request Apr 27, 2014
It looks this just requires taking out the checks.

I verified that, with the patch, I was able to run spark-shell through yarn without setting the environment variable.

Author: Sandy Ryza <[email protected]>

Closes apache#553 from sryza/sandy-spark-1053 and squashes the following commits:

b037676 [Sandy Ryza] SPARK-1053.  Don't require SPARK_YARN_APP_JAR
pdeyhim pushed a commit to pdeyhim/spark-1 that referenced this pull request Jun 25, 2014
It seems that the val "startTime" and "endTime" is never used, so delete them.

Author: WangTao <[email protected]>

Closes apache#553 from WangTaoTheTonic/master and squashes the following commits:

4fcb639 [WangTao] Delete the val that never used
gzm55 pushed a commit to MediaV/spark that referenced this pull request Jul 17, 2014
It looks this just requires taking out the checks.

I verified that, with the patch, I was able to run spark-shell through yarn without setting the environment variable.

Author: Sandy Ryza <[email protected]>

Closes apache#553 from sryza/sandy-spark-1053 and squashes the following commits:

b037676 [Sandy Ryza] SPARK-1053.  Don't require SPARK_YARN_APP_JAR
erikerlandson pushed a commit to erikerlandson/spark that referenced this pull request Nov 27, 2017
bzhaoopenstack added a commit to bzhaoopenstack/spark that referenced this pull request Sep 11, 2019
…e dev" (apache#553)

Change "make bin" to "make dev"
Now we just build the binary for local host env.

Close: theopenlab/openlab#247
MaxGekk pushed a commit to MaxGekk/spark that referenced this pull request Oct 2, 2026
The review of apache#553, stacked on this branch, found six more problems here. A count too low was unguarded - the walk would emit a node's subtree again with nothing to show it - so VarkaVectorWalk.emitValue now refuses a second visit of a node without a shared slot under CSE; no Varka suite, the fuzzers included, meets the refusal. Slots.fragmentKey takes the Analysis instead of a copy of its word algebra kept on every Slots. A group's loop and epilogue share one use count (Analysis.bodyUses, cleared with each grouping's materialized prefixes) where each made its own. The arithmetic test reads its slots through VarkaUnreadLocals rather than parsing disassembly. Prediction 4 of PLAN_TASK_223.md 9.2 kept its "Held" after the first review's count moved five coverage rows, and row 223 attached its removed-slot figures to the wrong sentence; both are corrected. No emitted byte moves.

Generated-by: Claude Code (Claude Opus 5.5)
MaxGekk added a commit to MaxGekk/spark that referenced this pull request Oct 2, 2026
### What changes were proposed in this pull request?

Task 223, one of milestone 6's size-control rows: a shared slot is now decided by how often the body itself uses a node, not by the kernel-wide count.

**The mechanism.** DAG-CSE in the vector walk computes a node used more than once at its first visit, then `dup`s it into a local that later visits load (`VarkaVectorWalk.emitValue`). `Slots.plan` decided which nodes get that local from `analysis.useCount`, the count over the whole kernel. A kernel is split into groups, each with its own loop and epilogue methods, so a node shared by outputs in different groups counted two or more although each group's body used it once: the body computed it, duplicated it and stored it into a local nothing read. That costs a `dup` and an `astore` - about three bytes - and a local; at run time C2 removes the dead store, so the gain is bytecode against the byte budget, plus the interpreter's and C1's work before C2. (The row expected a store and a reload; there is no reload.)

`Slots.plan` now counts how often the body itself visits each node, and gives a shared slot only above one. A node is visited once per output root it serves and once per edge the walk follows. Three kinds of edge are not followed:
- a calendar node's edge to its date, after the first node of its shared prefix fragment;
- that edge where the body loads the date's materialized prefix and does not visit the date;
- an `IsNotNull`'s edge to its column, whose validity word it reads but never its vector.

The count asks the emission's own rules for these rather than restating them (`Slots.bodyUses`, see the review below).

**How common it was, and what changes** (the cost corpus under the defaults, `PLAN_TASK_223.md` 2 and 9):

| family | shared slots used once in their body | loop + epilogue bytes |
|:--|--:|--:|
| wide, int lane | 66,260 of 86,108 | -0.66% |
| wide, long lane | 96,486 of 147,500 | -1.33% |
| fuzz, int lane | 2,136 of 5,394 | -0.14% |
| fuzz, long lane | 896 of 5,994 | -0.14% |
| size ladder, cheap tails | 0 | unchanged |

Most are column loads: a column three outputs read counts three kernel-wide and once in a group holding one of them. No build and no loop method moves in any family, and the methods of 8000 bytes and over fall from 169 to 155.

**Regenerated, reviewed:** `emitted_bytes.json` (every coverage row byte-identical; the fuzz blocks and option-arm digests move, as block digests over multi-group kernels must), the fitted price tables, and `emit_cost_audit.json` - no conclusion of `PLAN_TASK_199.md` 9 moves, the fitted model's 99th-percentile error at 2000 bytes and over going from 12.4% to 11.9%.

**The review** (`PLAN_TASK_223.md` 9.4) found nine problems, none of which changes an answer, and all are addressed. The first build's count followed two of those edges, so 5,722 shared slots over the corpus were still stored and never read, and the test that would have shown it, promised by the plan, was missing.
- The count now asks the walk's rule. To make that possible, the prefix fragment's key takes the word's owner in the kernel's word algebra, which is known before any slot is numbered and tells words apart as the slots do.
- The new `VarkaUnreadLocalsSuite` holds every loop and epilogue method of the cost corpus to no unread shared slot. Building it found the third edge, `IsNotNull`.
- No method of the corpus grows under the fix, and 2,692 shrink. `VarkaEmitterBudgetSuite`'s shared `HugeMethodLimit` crossings move later: 49 to 51 with the bitmap pass, and 44 to 45 without.
- The count no longer runs in the driver or with CSE off, `Analysis.useCount` is the set it had become, and the plan's risk section is corrected.

**A finding beyond the row**, recorded in `PLAN_TASK_223.md` 9.3, now row 239 and planned in apache#553: about 116,000 other reference locals in the corpus's loop and epilogue methods are stored and never read - validity segments every body's prologue still builds although task 70's bitmap pass writes most outputs' validity, and calendar prefix vectors a consuming group reloads every lane group (task 198) where its fields read a subset. Together about 3% of loop and epilogue bytes; their run-time cost is not measured yet.

Row 223 is marked done in `PLAN_MILESTONE_6.md`.

### Why are the changes needed?

Size control is a goal of this milestone: every byte the emitter writes without need counts against the byte budget a method is held to.

### Does this PR introduce _any_ user-facing change?

No. Generated code shrinks; no answer changes.

### How was this patch tested?

A new test in `VarkaEmitterArithmeticSuite`: two heavy outputs over one column, a group each, park the column nowhere, and two light outputs in one group still share it. With the kernel-wide count restored, the test fails, naming a dead slot in all eight bodies.

`VarkaUnreadLocalsSuite`: no shared slot of any loop or epilogue method in the cost corpus goes unread. It found 5,722 such slots before the review's fix, 396 with its first two edges fixed, and none now.

Every catalyst Varka suite: 503 passed, 0 failed, 27 canceled (all opt-ins: no hsdis disassembler here, the exhaustive sweeps, JFR, the option audit). `dev/scalastyle` and `dev/lint-java` pass. `dev/varka_quote_check.py` reports zero orphans and `dev/varka_precommit.sh` no findings.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5.5)
MaxGekk pushed a commit to MaxGekk/spark that referenced this pull request Oct 2, 2026
…k committed

The review of apache#553 found the plan's laptop timings traced to no committed results file. They now come from VarkaUnreadLocalsBenchmark and its results file, run on a quiet laptop: the classes as emitted and stripped by VarkaUnreadLocalsTrim, with a control for case order, and what one batch allocates. The masked ladder at 400 entries runs 8.6% faster stripped, nearly all of it the segments, and nothing else moves beyond its control; the dead segments are allocated in both bodies, 16 to 24 KB a batch on the ladder. Section 3.3 now decides an input's data segment by task 223's edge rule rather than skippedColumns, which cannot tell a column read for its validity from one read for its value; section 3.4 decides the epilogue's mask by a predicate over the body, since emitInRanges loops inside the lane group and a first reader does not dominate the rest; the census no longer calls 66,034 and 66,024 the same.

Generated-by: Claude Code (Claude Opus 5.5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants