[
https://issues.apache.org/jira/browse/IGNITE-28520?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Alex Abashev updated IGNITE-28520:
----------------------------------
Description:
h2. Summary
The CacheObject pre-marshal pass already runs on the user thread —
{{GridCacheIoManager.onSend}} has been calling {{msg.prepareMarshal(cctx)}} on
the sender thread since 2014 (commit by Anton Irinev, before {{ignite-63}}).
This ticket is *not* about moving work off the NIO worker; it is about
replacing the per-class hand-written CO traversal with a generated,
auto-invoked equivalent.
Today every {{GridCacheMessage}} subclass that carries {{CacheObject}} fields
has a hand-written {{prepareMarshal(GridCacheSharedContext)}} override that
walks those fields and calls {{co.prepareMarshal(ctx)}} on each. Adding a new
{{@Order CO}} field to such a class requires a matching update of the override.
Forgetting that update silently moves that field's {{Marshaller.marshal}} call
onto the NIO writer (it runs lazily inside {{writeTo}} when {{valBytes ==
null}}), which is the regression mode this ticket exists to prevent.
After this ticket lands, the same traversal is generated by APT and
auto-invoked from the framework — so coverage is structural rather than
maintained by hand.
h2. What changes
* {{MessageSerializer}} interface gets a new {{default void
prepareMarshalCacheObjects(M msg, CacheObjectValueContext ctx)}} (no-op
default).
* {{MessageSerializerGenerator}} (APT) emits this method on every concrete
serializer with at least one traversable {{@Order}} field; recurses into nested
concrete {{Message}} types via per-class {{*_SER}} static fields. Recursion is
blocked only for {{GridCacheIdMessage}} subclasses (top-level routable
messages) — helper messages with {{@Order int cacheId}} ({{GridCacheReturn}},
{{IgniteTxKey}}, {{IgniteTxEntry}}, {{GridCacheEntryInfo}}, {{UpdateErrors}})
are recursable.
* {{GridCacheIoManager.onSend}} adds
{{prepareMarshalGeneratedCacheObjects(msg)}} after the existing
{{msg.prepareMarshal(cctx)}}. Helper is gated on {{msg instanceof
GridCacheIdMessage}} and resolves {{cacheObjectContext}} from
{{((GridCacheIdMessage)msg).cacheId()}}. Both calls run on the user thread,
both are idempotent for any given {{CacheObject}} ({{if (valBytes == null)}}).
* Hand-written {{prepareMarshal(GridCacheSharedContext)}} overrides are removed
from {{GridCacheMessage}} subclasses where the body was a pure {{@Order}}-CO
traversal that the generator now performs:
** {{GridCacheTtlUpdateRequest}}, {{GridDhtForceKeysRequest}},
{{GridNearUnlockRequest}}, {{GridDhtUnlockRequest}},
{{GridDistributedLockRequest}}, {{GridDistributedLockResponse}},
{{GridDhtAtomicUpdateResponse}}, {{GridDhtAtomicNearResponse}},
{{GridNearGetRequest}}.
** Also dropped: {{ret.prepareMarshal(cctx)}} call inside
{{GridNearAtomicUpdateResponse.prepareMarshal}} (covered by nested-message
recursion in the generated serializer).
* Generator hygiene aligned with Ignite coding guidelines: {{private static
final}} modifier order, no trailing space after {{/**}} in class javadoc, no
trailing newline at EOF, empty line between top-level {{if}}-blocks in
{{prepareMarshalCacheObjects}}, empty line between map keys/values traversal
blocks.
h2. Why it is worth doing
* *Structural coverage.* New {{@Order CO}} fields are picked up automatically;
no risk of a missed override.
* *Less boilerplate.* Phase 2 cleanup deletes ~9 hand-written methods that were
doing the same loop the generator now writes.
* *One source of truth* for "which fields participate in pre-marshal traversal"
— the {{@Order}} annotations.
* *No behavioural change* for messages whose hand-written override stays — the
auto-invoke is a no-op on already-marshalled {{CacheObject}}s thanks to the
{{valBytes == null}} guard inside {{CacheObjectImpl.prepareMarshal}}.
h2. Out of scope
* Receive-side counterpart ({{finishUnmarshalCacheObjects}}) — split into a
separate ticket (IGNITE-YYYYY).
* Extending the auto-invoke to {{GridCacheMessage}} subclasses that are not
{{GridCacheIdMessage}} (e.g. {{GridDhtTxFinishResponse}}), or to nested
messages whose embedded {{cacheId}} differs from the outer's (e.g.
{{GridNearTxPrepareResponse}}'s {{retVal.cacheId()}}) — split into a separate
ticket (IGNITE-ZZZZZ). Until that lands, {{GridCacheReturn}} keeps its {{static
SER}} field and {{prepareMarshal(GridCacheContext)}} bridge for the two
responses that fall outside the auto-invoke gate.
h2. Acceptance criteria
* {{GridCacheIoManager.onSend}} calls
{{prepareMarshalGeneratedCacheObjects(msg)}} for every {{GridCacheIdMessage}}
subclass with a non-{{null}} {{cacheObjectContext}}.
* {{MessageProcessorTest}} green; fixture serializers byte-identical with
generator output.
* Targeted suites green: {{GridCacheReturnValueTransferSelfTest}},
{{IgniteCacheEntryProcessorCallTest}},
{{GridCachePartitionedTxMultiThreadedSelfTest}},
{{GridCacheRebalancingSyncSelfTest}} (force-keys path),
{{GridCachePutAllFailoverSelfTest}} (atomic update / unlock path),
{{IgniteCacheQueryMultiThreadedSelfTest}} (query/eviction path).
* Possible-blockers list from the TC bot for the PR contains no real
regressions (flakes only).
was:
h2. Summary
Move generic {{CacheObject}} marshalling for outgoing cache messages from the
NIO communication worker to the user thread, so the IO loop is no longer
blocked on serialisation work that has nothing to do with the wire protocol
itself.
The send path now does:
# {{msg.prepareMarshal(cctx)}} — existing hand-written hook on
{{GridCacheMessage}} subclasses (legacy);
# {{ser.prepareMarshalCacheObjects(msg, cacheObjCtx)}} — new auto-invoke of the
generated serializer's traversal of {{@Order}}-annotated {{CacheObject}} /
{{KeyCacheObject}} / nested {{Message}} fields.
Both run on the user (sender) thread inside {{GridCacheIoManager.onSend}}
before the message is enqueued for NIO; the NIO worker only does the actual
byte writes.
h2. Scope of this ticket
* New {{default}} method {{MessageSerializer#prepareMarshalCacheObjects(M,
CacheObjectValueContext)}} (no-op default).
* {{MessageProcessor}} / {{MessageSerializerGenerator}} emit this method on
every concrete serializer with at least one traversable {{@Order}} field;
recursion into nested concrete {{Message}} types via per-class {{*_SER}} static
fields.
* {{GridCacheIoManager.onSend}} calls
{{prepareMarshalGeneratedCacheObjects(msg)}} — gated on {{msg instanceof
GridCacheIdMessage}}, takes {{cacheObjectContext}} from
{{((GridCacheIdMessage)msg).cacheId()}}.
* Phase 2 cleanup: the hand-written {{prepareMarshal(GridCacheSharedContext)}}
override is removed from {{GridCacheMessage}} subclasses where its body was a
pure {{@Order}}-CO traversal that the generator now performs:
** {{GridCacheTtlUpdateRequest}}, {{GridDhtForceKeysRequest}},
{{GridNearUnlockRequest}}, {{GridDhtUnlockRequest}},
{{GridDistributedLockRequest}}, {{GridDistributedLockResponse}},
{{GridDhtAtomicUpdateResponse}}, {{GridDhtAtomicNearResponse}},
{{GridNearGetRequest}}.
** Also removed: {{ret.prepareMarshal(cctx)}} call in
{{GridNearAtomicUpdateResponse.prepareMarshal}} (covered by nested-message
recursion in the generated serializer).
* {{MessageSerializerGenerator.isRecursableMessage}} narrowed:
nested-{{Message}} recursion is blocked only for {{GridCacheIdMessage}}
subclasses (top-level routable). Helper messages with {{@Order int cacheId}}
({{GridCacheReturn}}, {{IgniteTxKey}}, {{IgniteTxEntry}},
{{GridCacheEntryInfo}}, {{UpdateErrors}}) become recursable.
* Generator hygiene: emits {{private static final}} (modern modifier order);
{{/**}} class javadoc starts without trailing space; no trailing newline; empty
line between top-level {{if}}-blocks in {{prepareMarshalCacheObjects}} per
Ignite coding guidelines.
h2. Out of scope
* Mirror work for the receive direction ({{finishUnmarshalCacheObjects}}) —
split into a separate ticket (IGNITE-YYYYY).
* Extending the auto-invoke beyond {{GridCacheIdMessage}} subclasses or to
messages whose retVal carries its own {{cacheId}} — split into a separate
ticket (IGNITE-ZZZZZ); blocks removal of the {{GridCacheReturn.SER}} bridge and
the explicit {{retVal.prepareMarshal(cctx)}} calls in
{{GridDhtTxFinishResponse}} / {{GridNearTxPrepareResponse}}.
h2. Acceptance criteria
* {{GridCacheIoManager.onSend}} invokes
{{prepareMarshalGeneratedCacheObjects(msg)}} for {{GridCacheIdMessage}}
subclasses with non-{{null}} {{cacheObjectContext}}.
* {{MessageProcessorTest}} green; fixture serializers byte-identical with
generator output.
* Targeted suites green: {{GridCacheReturnValueTransferSelfTest}},
{{IgniteCacheEntryProcessorCallTest}},
{{GridCachePartitionedTxMultiThreadedSelfTest}},
{{GridCacheRebalancingSyncSelfTest}} (force-keys path),
{{GridCachePutAllFailoverSelfTest}} (atomic update / unlock path),
{{IgniteCacheQueryMultiThreadedSelfTest}} (query/eviction path).
* No regression in possible-blockers from the TC bot for the PR.
> Move prepareMarshal / finishUnmarshal out of NIO communication thread — Phase
> 1: CacheObjects
> ---------------------------------------------------------------------------------------------
>
> Key: IGNITE-28520
> URL: https://issues.apache.org/jira/browse/IGNITE-28520
> Project: Ignite
> Issue Type: Task
> Reporter: Alex Abashev
> Assignee: Alex Abashev
> Priority: Minor
> Labels: IEP-132, ise
> Fix For: 2.19
>
> Time Spent: 3h 20m
> Remaining Estimate: 0h
>
> h2. Summary
> The CacheObject pre-marshal pass already runs on the user thread —
> {{GridCacheIoManager.onSend}} has been calling {{msg.prepareMarshal(cctx)}}
> on the sender thread since 2014 (commit by Anton Irinev, before
> {{ignite-63}}). This ticket is *not* about moving work off the NIO worker; it
> is about replacing the per-class hand-written CO traversal with a generated,
> auto-invoked equivalent.
> Today every {{GridCacheMessage}} subclass that carries {{CacheObject}} fields
> has a hand-written {{prepareMarshal(GridCacheSharedContext)}} override that
> walks those fields and calls {{co.prepareMarshal(ctx)}} on each. Adding a new
> {{@Order CO}} field to such a class requires a matching update of the
> override. Forgetting that update silently moves that field's
> {{Marshaller.marshal}} call onto the NIO writer (it runs lazily inside
> {{writeTo}} when {{valBytes == null}}), which is the regression mode this
> ticket exists to prevent.
> After this ticket lands, the same traversal is generated by APT and
> auto-invoked from the framework — so coverage is structural rather than
> maintained by hand.
> h2. What changes
> * {{MessageSerializer}} interface gets a new {{default void
> prepareMarshalCacheObjects(M msg, CacheObjectValueContext ctx)}} (no-op
> default).
> * {{MessageSerializerGenerator}} (APT) emits this method on every concrete
> serializer with at least one traversable {{@Order}} field; recurses into
> nested concrete {{Message}} types via per-class {{*_SER}} static fields.
> Recursion is blocked only for {{GridCacheIdMessage}} subclasses (top-level
> routable messages) — helper messages with {{@Order int cacheId}}
> ({{GridCacheReturn}}, {{IgniteTxKey}}, {{IgniteTxEntry}},
> {{GridCacheEntryInfo}}, {{UpdateErrors}}) are recursable.
> * {{GridCacheIoManager.onSend}} adds
> {{prepareMarshalGeneratedCacheObjects(msg)}} after the existing
> {{msg.prepareMarshal(cctx)}}. Helper is gated on {{msg instanceof
> GridCacheIdMessage}} and resolves {{cacheObjectContext}} from
> {{((GridCacheIdMessage)msg).cacheId()}}. Both calls run on the user thread,
> both are idempotent for any given {{CacheObject}} ({{if (valBytes == null)}}).
> * Hand-written {{prepareMarshal(GridCacheSharedContext)}} overrides are
> removed from {{GridCacheMessage}} subclasses where the body was a pure
> {{@Order}}-CO traversal that the generator now performs:
> ** {{GridCacheTtlUpdateRequest}}, {{GridDhtForceKeysRequest}},
> {{GridNearUnlockRequest}}, {{GridDhtUnlockRequest}},
> {{GridDistributedLockRequest}}, {{GridDistributedLockResponse}},
> {{GridDhtAtomicUpdateResponse}}, {{GridDhtAtomicNearResponse}},
> {{GridNearGetRequest}}.
> ** Also dropped: {{ret.prepareMarshal(cctx)}} call inside
> {{GridNearAtomicUpdateResponse.prepareMarshal}} (covered by nested-message
> recursion in the generated serializer).
> * Generator hygiene aligned with Ignite coding guidelines: {{private static
> final}} modifier order, no trailing space after {{/**}} in class javadoc, no
> trailing newline at EOF, empty line between top-level {{if}}-blocks in
> {{prepareMarshalCacheObjects}}, empty line between map keys/values traversal
> blocks.
> h2. Why it is worth doing
> * *Structural coverage.* New {{@Order CO}} fields are picked up
> automatically; no risk of a missed override.
> * *Less boilerplate.* Phase 2 cleanup deletes ~9 hand-written methods that
> were doing the same loop the generator now writes.
> * *One source of truth* for "which fields participate in pre-marshal
> traversal" — the {{@Order}} annotations.
> * *No behavioural change* for messages whose hand-written override stays —
> the auto-invoke is a no-op on already-marshalled {{CacheObject}}s thanks to
> the {{valBytes == null}} guard inside {{CacheObjectImpl.prepareMarshal}}.
> h2. Out of scope
> * Receive-side counterpart ({{finishUnmarshalCacheObjects}}) — split into a
> separate ticket (IGNITE-YYYYY).
> * Extending the auto-invoke to {{GridCacheMessage}} subclasses that are not
> {{GridCacheIdMessage}} (e.g. {{GridDhtTxFinishResponse}}), or to nested
> messages whose embedded {{cacheId}} differs from the outer's (e.g.
> {{GridNearTxPrepareResponse}}'s {{retVal.cacheId()}}) — split into a separate
> ticket (IGNITE-ZZZZZ). Until that lands, {{GridCacheReturn}} keeps its
> {{static SER}} field and {{prepareMarshal(GridCacheContext)}} bridge for the
> two responses that fall outside the auto-invoke gate.
> h2. Acceptance criteria
> * {{GridCacheIoManager.onSend}} calls
> {{prepareMarshalGeneratedCacheObjects(msg)}} for every {{GridCacheIdMessage}}
> subclass with a non-{{null}} {{cacheObjectContext}}.
> * {{MessageProcessorTest}} green; fixture serializers byte-identical with
> generator output.
> * Targeted suites green: {{GridCacheReturnValueTransferSelfTest}},
> {{IgniteCacheEntryProcessorCallTest}},
> {{GridCachePartitionedTxMultiThreadedSelfTest}},
> {{GridCacheRebalancingSyncSelfTest}} (force-keys path),
> {{GridCachePutAllFailoverSelfTest}} (atomic update / unlock path),
> {{IgniteCacheQueryMultiThreadedSelfTest}} (query/eviction path).
> * Possible-blockers list from the TC bot for the PR contains no real
> regressions (flakes only).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
