[
https://issues.apache.org/jira/browse/IGNITE-26630?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Anton Vinogradov updated IGNITE-26630:
--------------------------------------
Description:
h3. What partition() actually is
*{{GridCacheMessage#partition()}}* is not a data partition. It is the key that
picks a stripe: {{GridIoMessage#partition()}} reads it
(GridIoMessage.java:183-190), and {{GridIoManager}} hands the message to the
striped executor when the value is not {{STRIPE_DISABLED_PART}}
(GridIoManager.java:1392-1401).
So the method cannot simply be removed. The base class returns {{-1}}
(GridCacheMessage.java:129) and {{-1}} is not {{STRIPE_DISABLED_PART}}
({{Integer.MIN_VALUE}}, GridIoMessage.java:38), so even a message that does not
care goes to the striped pool. Removing the method would move every cache
message to the plain system pool. Functional tests would not notice; only a
benchmark would.
h3. Step 1: no wire change
Replace the {{instanceof}} chain in {{GridIoMessage#partition()}} with an
interface, and name the method for what it is — a stripe key, not a partition.
The chain covers {{GridCacheMessage}} and {{DataStreamerRequest}} today and
returns {{STRIPE_DISABLED_PART}} for everything else.
h3. Step 2: wire change, so it must land before 2.19
Five responses carry a field on the wire only to pick a stripe. A response
echoes the partition of its request so that it lands on the same stripe, and
nothing on the receiving side reads the value.
|| class || field || note ||
| GridDistributedTxPrepareResponse | {{@Order(1) part}} | |
| GridDistributedTxFinishResponse | {{@Order(2) part}} | {{stripeIdx()}} is
final, subclasses pass the value to super |
| GridDhtAtomicNearResponse | {{@Order(0) partId}} | |
| GridDhtAtomicUpdateResponse | {{@Order(3) partId}} | |
| GridNearAtomicUpdateResponse | {{@Order(5) partId}} | |
Dropping a field means the message is no longer pinned to the stripe of its
request. Nothing reads the value, but work moves between stripes, so each class
needs its own answer to "is losing that pinning safe here". This step needs a
benchmark: tests stay green either way.
{{DataStreamerRequest}} {{@Order(15) partId}} looks like a sixth candidate but
is not one. It is not an echo: DataStreamerImpl.java:2005 puts either the
stripe or {{NO_STRIPE}} there, depending on the receiver, so the value carries
a decision. The receiver cannot make that decision itself, because the stripe
is picked before the payload is read — the receiver identity lives in
{{updaterBytes}} and unmarshalling it on the NIO thread is exactly what should
not happen. Removing this field needs a separate answer.
h3. Out of scope
Three fields look the same and must stay:
* {{GridCacheQueryRequest}} {{@Order(18) part}} — a real scan partition. It
comes from the user and is read at GridCacheDistributedQueryManager.java:251.
* {{GridNearAtomicCheckUpdateRequest}} {{@Order(0) partId}} — read at
GridDhtAtomicCache.java:3244 and copied into the response; the primary cannot
restore it from anything else.
* {{GridJobExecuteRequest}} {{@Order(22) part}} — the class is not a
{{GridCacheMessage}}, so it never reaches the striping chain. The value is an
affinity partition, read at GridJobProcessor.java:1188.
Messages that compute the value need no change: {{GridDhtTxPrepareRequest}},
{{GridDhtTxFinishRequest}}, {{GridNearTxPrepareRequest}} and
{{GridNearTxFinishRequest}} use {{U.safeAbs(version().hashCode())}};
{{GridDistributedLockRequest}} and {{GridNearUnlockRequest}} take it from the
first key; {{GridDhtPartitionsAbstractMessage}} returns
{{STRIPE_DISABLED_PART}}.
h3. One thing to keep in mind
For DHT atomic updates the stripe carries correctness, not only speed: the
deferred response buffer is a {{ThreadLocal}} (GridDhtAtomicCache.java:166-171)
and the timeout flushes it back into the same stripe (:3444, :3493). The
contract is guarded by {{assert
Thread.currentThread().getName().startsWith("sys-stripe-")}} (:3259) — an
assert on a thread name, which does nothing unless the JVM runs with {{-ea}}.
That path is on the request side, where the stripe key is computed from the
keys, so step 2 does not touch it.
was:
h3. What partition() actually is
*{{GridCacheMessage#partition()}}* is not a data partition. It is the key that
picks a stripe: {{GridIoMessage#partition()}} reads it
(GridIoMessage.java:183-190), and {{GridIoManager}} hands the message to the
striped executor when the value is not {{STRIPE_DISABLED_PART}}
(GridIoManager.java:1392-1401).
So the method cannot simply be removed. The base class returns {{-1}}
(GridCacheMessage.java:129) and {{-1}} is not {{STRIPE_DISABLED_PART}}
({{Integer.MIN_VALUE}}, GridIoMessage.java:38), so even a message that does not
care goes to the striped pool. Removing the method would move every cache
message to the plain system pool. Functional tests would not notice; only a
benchmark would.
h3. Step 1: no wire change
Replace the {{instanceof}} chain in {{GridIoMessage#partition()}} with an
interface, and name the method for what it is — a stripe key, not a partition.
The chain covers {{GridCacheMessage}} and {{DataStreamerRequest}} today and
returns {{STRIPE_DISABLED_PART}} for everything else.
h3. Step 2: wire change, so it must land before 2.19
Six messages carry a field on the wire only to pick a stripe. A response echoes
the partition of its request so that it lands on the same stripe, and nothing
on the receiving side reads the value.
|| class || field || note ||
| GridDistributedTxPrepareResponse | {{@Order(1) part}} | |
| GridDistributedTxFinishResponse | {{@Order(2) part}} | {{partition()}} is
final, subclasses pass the value to super |
| GridDhtAtomicNearResponse | {{@Order(0) partId}} | |
| GridDhtAtomicUpdateResponse | {{@Order(3) partId}} | |
| GridNearAtomicUpdateResponse | {{@Order(5) partId}} | the setter
{{partition(int)}} at :365 has no callers at all |
| DataStreamerRequest | {{@Order(15) partId}} | the value is the sender stripe
index, not a data partition; DataStreamerImpl.java:2005 sends
{{STRIPE_DISABLED_PART}} unless the receiver is the isolated updater |
Dropping a field means the message is no longer pinned to the stripe of its
request. Nothing reads the value, but work moves between stripes, so each class
needs its own answer to "is losing that pinning safe here". This step needs a
benchmark: tests stay green either way.
h3. Out of scope
Three fields look the same and must stay:
* {{GridCacheQueryRequest}} {{@Order(18) part}} — a real scan partition. It
comes from the user and is read at GridCacheDistributedQueryManager.java:251.
* {{GridNearAtomicCheckUpdateRequest}} {{@Order(0) partId}} — read at
GridDhtAtomicCache.java:3244 and copied into the response; the primary cannot
restore it from anything else.
* {{GridJobExecuteRequest}} {{@Order(22) part}} — the class is not a
{{GridCacheMessage}}, so it never reaches the striping chain. The value is an
affinity partition, read at GridJobProcessor.java:1188.
Messages that compute the value need no change: {{GridDhtTxPrepareRequest}},
{{GridDhtTxFinishRequest}}, {{GridNearTxPrepareRequest}} and
{{GridNearTxFinishRequest}} use {{U.safeAbs(version().hashCode())}};
{{GridDistributedLockRequest}} and {{GridNearUnlockRequest}} take it from the
first key; {{GridDhtPartitionsAbstractMessage}} returns
{{STRIPE_DISABLED_PART}}.
h3. One thing to keep in mind
For DHT atomic updates the stripe carries correctness, not only speed: the
deferred response buffer is a {{ThreadLocal}} (GridDhtAtomicCache.java:166-171)
and the timeout flushes it back into the same stripe (:3444, :3493). The
contract is guarded by {{assert
Thread.currentThread().getName().startsWith("sys-stripe-")}} (:3259) — an
assert on a thread name, which does nothing unless the JVM runs with {{-ea}}.
That path is on the request side, where the stripe key is computed from the
keys, so step 2 does not touch it.
> Investigate removal of GridCacheMessage#partition
> -------------------------------------------------
>
> Key: IGNITE-26630
> URL: https://issues.apache.org/jira/browse/IGNITE-26630
> Project: Ignite
> Issue Type: Task
> Reporter: Ilya Shishkov
> Assignee: Anton Vinogradov
> Priority: Minor
> Labels: IEP-132, ise, wire-format
> Fix For: 2.19
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> h3. What partition() actually is
> *{{GridCacheMessage#partition()}}* is not a data partition. It is the key
> that picks a stripe: {{GridIoMessage#partition()}} reads it
> (GridIoMessage.java:183-190), and {{GridIoManager}} hands the message to the
> striped executor when the value is not {{STRIPE_DISABLED_PART}}
> (GridIoManager.java:1392-1401).
> So the method cannot simply be removed. The base class returns {{-1}}
> (GridCacheMessage.java:129) and {{-1}} is not {{STRIPE_DISABLED_PART}}
> ({{Integer.MIN_VALUE}}, GridIoMessage.java:38), so even a message that does
> not care goes to the striped pool. Removing the method would move every cache
> message to the plain system pool. Functional tests would not notice; only a
> benchmark would.
> h3. Step 1: no wire change
> Replace the {{instanceof}} chain in {{GridIoMessage#partition()}} with an
> interface, and name the method for what it is — a stripe key, not a
> partition. The chain covers {{GridCacheMessage}} and {{DataStreamerRequest}}
> today and returns {{STRIPE_DISABLED_PART}} for everything else.
> h3. Step 2: wire change, so it must land before 2.19
> Five responses carry a field on the wire only to pick a stripe. A response
> echoes the partition of its request so that it lands on the same stripe, and
> nothing on the receiving side reads the value.
> || class || field || note ||
> | GridDistributedTxPrepareResponse | {{@Order(1) part}} | |
> | GridDistributedTxFinishResponse | {{@Order(2) part}} | {{stripeIdx()}} is
> final, subclasses pass the value to super |
> | GridDhtAtomicNearResponse | {{@Order(0) partId}} | |
> | GridDhtAtomicUpdateResponse | {{@Order(3) partId}} | |
> | GridNearAtomicUpdateResponse | {{@Order(5) partId}} | |
> Dropping a field means the message is no longer pinned to the stripe of its
> request. Nothing reads the value, but work moves between stripes, so each
> class needs its own answer to "is losing that pinning safe here". This step
> needs a benchmark: tests stay green either way.
> {{DataStreamerRequest}} {{@Order(15) partId}} looks like a sixth candidate
> but is not one. It is not an echo: DataStreamerImpl.java:2005 puts either the
> stripe or {{NO_STRIPE}} there, depending on the receiver, so the value
> carries a decision. The receiver cannot make that decision itself, because
> the stripe is picked before the payload is read — the receiver identity lives
> in {{updaterBytes}} and unmarshalling it on the NIO thread is exactly what
> should not happen. Removing this field needs a separate answer.
> h3. Out of scope
> Three fields look the same and must stay:
> * {{GridCacheQueryRequest}} {{@Order(18) part}} — a real scan partition. It
> comes from the user and is read at GridCacheDistributedQueryManager.java:251.
> * {{GridNearAtomicCheckUpdateRequest}} {{@Order(0) partId}} — read at
> GridDhtAtomicCache.java:3244 and copied into the response; the primary cannot
> restore it from anything else.
> * {{GridJobExecuteRequest}} {{@Order(22) part}} — the class is not a
> {{GridCacheMessage}}, so it never reaches the striping chain. The value is an
> affinity partition, read at GridJobProcessor.java:1188.
> Messages that compute the value need no change: {{GridDhtTxPrepareRequest}},
> {{GridDhtTxFinishRequest}}, {{GridNearTxPrepareRequest}} and
> {{GridNearTxFinishRequest}} use {{U.safeAbs(version().hashCode())}};
> {{GridDistributedLockRequest}} and {{GridNearUnlockRequest}} take it from the
> first key; {{GridDhtPartitionsAbstractMessage}} returns
> {{STRIPE_DISABLED_PART}}.
> h3. One thing to keep in mind
> For DHT atomic updates the stripe carries correctness, not only speed: the
> deferred response buffer is a {{ThreadLocal}}
> (GridDhtAtomicCache.java:166-171) and the timeout flushes it back into the
> same stripe (:3444, :3493). The contract is guarded by {{assert
> Thread.currentThread().getName().startsWith("sys-stripe-")}} (:3259) — an
> assert on a thread name, which does nothing unless the JVM runs with {{-ea}}.
> That path is on the request side, where the stripe key is computed from the
> keys, so step 2 does not touch it.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
