[srusia-delphix]

Problem

Jira: DLPX-99491

Delimited-file masking on Hop 2.15 is ~3x slower than on Hop 1.2.0. Measured on 1M rows over SFTP, same job configuration and the same five algorithms on both engines:

	2026.6 + Hop 2.15	2026.4 + Hop 1.2.0
wall clock	49.8s	16.5s
reader throughput	7.58 MB/s	23.56 MB/s

Database connectors are unaffected, and this is not engine-version drift: 2026.6 matched 2026.4 before the Hop upgrade, so the regression comes from the Hop version change itself.

Reproduce

1. Masking job over a delimited file whose format declares a header or trailer record (the engine then generates a FilterRows transform whose condition compares a field against a String constant).
2. Mask 1M rows.
3. Expected: throughput comparable to Hop 1.2.0. Actual: ~3x slower, and the FilterRows transform consumes a full CPU core for the whole run.

Any FilterRows condition with a constant right value is affected; the masking engine is simply a workload that evaluates one per row.

Evaluation

Hop 2.15 refactored org.apache.hop.core.Condition. In 1.2.0 the right-hand side of a condition was a pre-built ValueMetaAndData rightExact field, and evaluate() made zero createValueMeta calls. In 2.15 it became a CValue rightValue, and evaluate() constructs fresh metadata on every row, at two sites:

• Condition.java:359 — rightValue.createValueMeta()
• Condition.java:364 — rightValue.createValueData(), which builds a ValueMetaAndData, i.e. precisely the object 1.2.0 cached once

Each construction runs ValueMetaBase.<init>, which reads two system properties (HOP_LENIENT_STRING_TO_NUMBER_CONVERSION and HOP_EMPTY_STRING_DIFFERS_FROM_NULL). createValueData() builds three ValueMeta objects internally, so a single row can perform six property reads. Under a SecurityManager each read is a full policy evaluation:

java.net.URLDecoder.decode
  CustomPolicy.getDomainSourcePath(CustomPolicy.java:127)
  CustomPolicy.implies(CustomPolicy.java:83)
  java.security.AccessController.checkPermission
  CustomSecurityManager.checkPermission(CustomSecurityManager.java:30)
  java.lang.System.getProperty(System.java:965)
  ValueMetaBase.<init>(ValueMetaBase.java:336)
  ValueMetaString.<init>(ValueMetaString.java:32)
  PluginRegistry.loadClass(PluginRegistry.java:466)     <- reflective newInstance
  ValueMetaFactory.createValueMeta(ValueMetaFactory.java:51)
  Condition$CValue.createValueMeta(Condition.java:835)
  Condition.evaluate(Condition.java:359)
  FilterRows.keepRow(FilterRows.java:50)                <- synchronized
  FilterRows.processRow(FilterRows.java:121)

So every row pays reflective class instantiation plus several policy evaluations, and because keepRow is synchronized the cost cannot be diluted with additional transform copies.

Profiling evidence (jstack sampling the job JVM on both engines; 135 samples on 2.15, 61 on 1.2.0)

On 2.15 the generated Re-route Header and Trailer FilterRows transform burns 46.75s of CPU across a 49.8s run — a fully pegged core, 2.5x the masking algorithm's own 18.3s — and is RUNNABLE 96%. On 1.2.0 the same transform is TIMED_WAITING 97%, doing essentially nothing.

Share of that transform's samples on 2.15:

inside either per-row CValue call	91.2%
CustomPolicy.getDomainSourcePath (URLDecoder + Paths.get)	39.8%
AccessController stack walk (JVM-side)	24.8%
reflection / PluginRegistry.loadClass	47.8%
genuinely waiting for rows	4.4%

Distinctive frame counts, 2.15 vs 1.2.0: FilterRows.keepRow 106 vs 1, createValueMeta 175 vs 0, CustomPolicy 97 vs 0, URLDecoder.decode 78 vs 0.

Independently confirmed by removing the trigger rather than the cost: with a file format carrying no header/trailer record — which stops the engine emitting the constant-valued condition — 2.15 drops from 49.8s to 24s and the algorithm reaches Run 97% / InputWait 2% with 4 worker threads, the saturated steady state 1.2.0 shows. With the same algorithms assigned on both engines, the 3x gap collapses to 1.2.0 being only ~10% faster.

Solution

Condition.evaluate() now calls getCachedValueMeta() / getCachedValueData() instead of the create* methods. Both results derive only from CValue's fields — name, type, text, length, precision, nullValue, mask — so they are built once and reused.

CValue is annotated @Getter @Setter, so those fields are publicly mutable. The cache therefore records the field values it was derived from and rebuilds whenever any of them differ:

private boolean describes(CValue value) {
  return length == value.length
      && precision == value.precision
      && nullValue == value.nullValue
      && Objects.equals(name, value.name)
      && Objects.equals(type, value.type)
      && Objects.equals(text, value.text)
      && Objects.equals(mask, value.mask);
}

Keying on the values rather than invalidating inside setters also covers the reflective field writes @HopMetadataProperty performs during deserialization, and it leaves the public API and the one-line class-level @Setter intact. The guard is cheap relative to what it protects: seven primitive/reference comparisons per row, against the six System.getProperty calls plus reflective instantiation it avoids.

Four deliberate design points:

1. createValueMeta() and createValueData() are left public and uncached. Their only external callers are ConditionEditor:355, which passes the result to EnterValueDialog (a dialog that may modify it), and ConditionEditor:948. Caching those would risk the UI corrupting a shared instance, so the cached accessors are package-private and used only on the per-row path.

2. The cache field is volatile, because a Condition is shared. FilterRows.keepRow reads meta.getCondition() — one instance across every copy of the transform — and each copy's synchronized is on its own FilterRows, so it does not protect the Condition. (See the existing TODO at FilterRows:163 about cloning the condition into data; evaluate() already mutates leftFieldIndex / rightFieldIndex / rightValue on that shared object.) A duplicate build is harmless since both instances are equivalent; a half-published one would not be.

3. @Getter(AccessLevel.NONE) @Setter(AccessLevel.NONE) keeps Lombok off the cache field, so internal cache state is not published as public API.

4. The value data is built lazily rather than alongside the metadata. evaluate() only needs it when the condition has no right field, so building it eagerly could surface a conversion error on a path that previously never ran. Its flag is stored after the value, so any thread observing the flag also observes the value.

No change to generated pipelines or to on-disk metadata, so there is no routing or serialization risk.

Testing Done

Build and unit tests — hop-core, JDK 17:

check	result
mvn -pl core -am compile	clean
ConditionTest	10 tests, 0 failures (5 existing + 5 added), stable over 3 consecutive runs
ValueMetaBaseTest + ValueMetaBaseLenientStringConversionTest	71 tests, 0 failures
full hop-core suite	622 tests, 0 failures, 0 errors, 10 pre-existing skips, BUILD SUCCESS
mvn -pl core spotless:check	clean
CValue public API (javap)	no cache accessors generated; only the 7 data setters remain

mvn -pl core -am -DskipTests compile
mvn -pl core test
mvn -pl core spotless:check

Added coverage. ConditionTest previously exercised the constant-right-value path only once, and never across multiple rows — which is exactly what this change affects. Five tests were added. Only the first guards the performance contract; the rest guard correctness and the UI contract:

• testEvaluateReusesTheRightValueAcrossRows — a left IValueMeta subclass captures what evaluate() passes into compare(data1, meta2, data2) across two rows, asserting assertSame on both right-side instances. This is the test that stops the regression silently returning: with evaluate() reverted to create* per row it fails, while all four other new tests still pass.

• testConstantRightValueStaysCorrectAcrossRows — evaluates one constant-valued condition against four different rows, so a stale or shared value producing a wrong answer would fail.

• testConstantRightValueIsBuiltOnlyOnce — assertSame on repeated accessor calls, plus assertNotSame against createValueMeta() to hold the contract that UI callers still get fresh objects. Note this guards the accessors, not evaluate(). Uses 10000L rather than a small value so Long autoboxing cannot make assertSame pass spuriously.

• testConstantRightValueIsRebuiltAfterMutation — mutates through setText and setNullValue and asserts the evaluation result changes, then asserts per field that each of the seven values describes() compares (name, type, text, length, precision, nullValue, mask) replaces the cached instance on its own. The per-field checks assert instance identity rather than outcome, because changing name, length, precision or mask does not alter the comparison result. Verified by dropping fields from describes() one at a time — each failure names the culprit. Verified to be a real guard rather than one that passes by construction: with describes() temporarily forced to return true, it fails.

• testCachedRightValueTolerateConcurrentColdAccess — 8 threads released onto a genuinely cold cache via a two-latch barrier (workers signal arrival; the gate only opens once all have parked, since pool threads start lazily), 500 iterations each, with Future.get() rethrowing anything raised on a worker. Covers concurrent first initialisation and the lazy valueDataBuilt path under contention. It deliberately drives the cache accessors rather than evaluate(), which writes leftFieldIndex, rightFieldIndex, rightValue, inList and rightString on the shared instance and so is not thread safe regardless of caching — see Notes to Reviewers.

Still outstanding — end-to-end performance has not yet been measured with this build. The root cause and the trigger are both confirmed by measurement (see Evaluation), but validating the fix itself needs a published Hop artifact for dms-core-gate to consume. Plan:

1. Publish this branch to masking-maven under a distinct version (e.g. 2.15.0-dlpx1) rather than re-publishing 2.15.0, so the before/after is unambiguous. Note the full reactor is required, including hop-client-<version>.zip, which HopAdaptor consumes.
2. Bump hopVersion in dms-core-gate/gradle.properties and re-run the 1M job.
3. Pass criterion is stronger than wall clock: Re-route Header and Trailer should fall from 46.75s CPU / 96% RUNNABLE to near-idle, and the algorithm transform should reach ~95% RUNNABLE, matching 1.2.0's steady state.

Notes to Reviewers

Why not fix ValueMetaBase instead? That was the first candidate — hoisting the two System.getProperty reads out of the constructor into statics would have removed the cost for every caller, not just Condition. It is not viable: four test classes plus a JUnit rule set those properties and expect the next constructed ValueMeta to observe them — ValueMetaBaseLenientStringConversionTest (toggles Y/N across 4 tests), ValueMetaBaseTest:319,337 (N then Y within one test), EditRowsDialog_EmptyStringVsNull_Test, and RestoreHopEnvironment, which clears both between tests. Static caching would freeze the value at class-load and silently break all of them. Fixing Condition addresses the actual regression and leaves ValueMetaBase semantics untouched, which is why its 71 tests still pass.

Upstream. This is an upstream Hop regression, not Delphix-specific: any Hop user with a constant-valued FilterRows condition pays it, and the SecurityManager only amplifies an already-wasteful per-row allocation. Worth reporting to Apache Hop so the fork can eventually drop the patch.

Jira ID in the code comment follows the convention of the existing Delphix patches in this fork (DLPX-46230 / DLPX-82789 in ValueMetaBase, DLPX-51415 in the Sybase block) — it tells whoever next forward-ports the patch why the hunk exists.

evaluate() is already not thread safe, independent of this change. It writes five shared, non-volatile fields on the shared instance: leftFieldIndex (:336), rightFieldIndex (:342), rightValue = null (:345, destructive), inList (:420) and rightString (:451). FilterRows.keepRow is synchronized on its own FilterRows instance, which serialises rows within a copy but not across copies, so a FilterRows with copies > 1 already races on those fields — see the existing TODO at FilterRows:163. That is why the new concurrency test drives the cache accessors rather than evaluate(): a test over full evaluate() would be exercising that pre-existing race and would fail or flake for reasons unrelated to this PR. Looks worth its own issue as a latent correctness bug; happy to file it.

Limits of the concurrency test. It cannot prove the absence of a race, and an unsafe publication would very likely still pass on x86. The safety argument rests on the cache field being volatile and the lazy flag being written after the value; the test covers concurrent first initialisation, the lazy path under contention, and gross errors such as returning a holder before its value is set.

Follow-up, not in scope here. The masking engine also generates a FilterRows per record type for multi-record-type files, using function REGEXP. That path is worse per row, because Pattern.matches recompiles the pattern on every call in addition to building the metadata. This change fixes the metadata half of it; the recompilation is a separate issue.

[Copilot]

Copilot review overview

🟡 Changes recommended

Existing generated setters can leave cached values stale and expose internal cache state publicly.

Review effort: Balanced
Findings: 1

Open (1)

• Invalidate cached data and metadata when CValue fields change · New

What changed in this PR

Caches constant condition metadata/data to eliminate per-row construction in Condition.evaluate().

Changes:

• Adds thread-visible lazy caches for constant right-hand values.
• Adds correctness and cache-reuse tests.

File	Description
core/​src/​main/​java/​org/​apache/​hop/​core/​Condition.java	Reuses cached constant metadata and data during evaluation.
core/​src/​test/​java/​org/​apache/​hop/​core/​ConditionTest.java	Tests repeated evaluation and cache reuse.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Concurrent access to the newly shared lazy cache lacks test coverage.

Review effort: Balanced
Findings: 1

Open (1)

• Add concurrent cold-cache test for shared transform cache · New

Resolved since last review (1)

• Invalidate cached data and metadata when CValue fields change

[Copilot]

Copilot review overview

🟡 Changes recommended

The tests do not verify that repeated evaluate() calls actually reuse cached values.

Review effort: Balanced
Findings: 1

Open (1)

• Test does not verify metadata/data reuse in evaluate() · New

Resolved since last review (1)

• Add concurrent cold-cache test for shared transform cache

[Copilot]

Copilot review overview

🔵 Needs a closer look

The concurrency test does not ensure all workers are ready before releasing the cold-cache access.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Test does not verify metadata/data reuse in evaluate()

[zach-frank-delphix]

Interesting approach - I think we should expand the new tests, and I think we should gate this on results of end to end testing as well.

Can you also please file/ link the follow up tickets here for posterity?

[sandeep-delphix]

Changes looks good and comments are covered already.

[AnoopKumarDelphix]

Overall looks good. The root-cause analysis is thorough, the fix is narrowly scoped, and the cache-invalidation and safe-publication reasoning holds up. Approving, with a few follow-ups:

1. Stress/performance results: The Testing section says end-to-end performance hasn't been measured with this build yet. Please run the 1M-row delimited-file masking job against the published artifact (e.g. 2.15.0-dlpx1) and add the before/after numbers to the description: wall clock, reader throughput, and CPU and RUNNABLE % for Re-route Header and Trailer.
2. Multi-copy benchmark: Was a FilterRows with copies > 1 benchmarked when the field type differs from the constant's type (e.g. a String field compared to a Number constant)? Every copy now shares the one cached ValueMeta. Its conversion methods (convertNumberToString, convertDateToString, etc.) are synchronized, so those copies now compete for one lock where each row used to get its own object. This should still beat building a new object per row, but it would be good to confirm with numbers.
3. If contention shows up: A per-thread cache instead of a single shared instance would avoid it. I don't think that's needed unless the benchmark shows a problem.

[srusia-delphix]

This patch on 2.15.0 is not needed as this issue is already fixed by upstream in 2.17.0.