[jira] [Commented] (IGNITE-28356) Get rid of redundant wire companion fields and hand-written marshalling in messages

["Anton Vinogradov (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/IGNITE-28356?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18100076#comment-18100076 ] 

Anton Vinogradov commented on IGNITE-28356:
-------------------------------------------

[Ignite PR Checker|https://ignite-pr-checker.is-a.dev/?pr=13408] verdict for PR 13408 · RunAll build [9236765|https://ci2.ignite.apache.org/build/9236765] · 147 suites ran, 0 reused

(x) *2 blocker(s) in 2 suite(s):*
- Queries 5: {{org.apache.ignite.testsuites.IgniteBinaryCacheQueryTestSuite5: org.apache.ignite.internal.processors.cache.index.DynamicEnableIndexingBasicSelfTest.testEnableDynamicIndexing[hasNear=false,nodeIdx=3,cacheMode=PARTITIONED,atomicityMode=ATOMIC]}}
- Cache 2: {{org.apache.ignite.testsuites.IgniteCacheTestSuite2: org.apache.ignite.internal.processors.cache.distributed.near.GridNearTxPrepareDestroyedCacheTest.testPrepareOfDestroyedCacheAnsweredWithErrorResponse}}


⏳ _Auto re-run *#1* in progress — 2 blocker suite(s) re-queued (attempt 1/2). This comment updates when they settle._

> Get rid of redundant wire companion fields and hand-written marshalling in messages
> -----------------------------------------------------------------------------------
>
>                 Key: IGNITE-28356
>                 URL: https://issues.apache.org/jira/browse/IGNITE-28356
>             Project: Ignite
>          Issue Type: Task
>            Reporter: Ilya Shishkov
>            Assignee: Anton Vinogradov
>            Priority: Minor
>              Labels: IEP-132, ise
>             Fix For: 2.19
>
>          Time Spent: 14h 50m
>  Remaining Estimate: 0h
>
> The message codegen (IEP-132) can marshal a logical field into its wire companion by itself, yet many
> messages still keep both the field and a hand-written \{{marshal()}}/\{{unmarshal()}} pair that fills the
> companion. Some of those pairs carry no logic at all, and some of the companions describe data that is
> never sent. Each section below is a problem found and the change that closes it.
> h3. A map whose values are always null
> {\{GridDistributedTxPrepareRequest#dhtVers}} was a map plus two companions. All three producers put
> {\{null}} as the value, so the consumer's \{{ver.getValue() == null || !equals}} is always true.
> *Fix:* the field becomes \{{Collection<IgniteTxKey> dhtVerKeys}}. The value companion is gone and the
> request is \{{4 + 2N}} bytes shorter.
> h3. A key travelling next to its own value
> {\{GridNearTxPrepareResponse#ownedVals}} was a map plus two companions, and \{{CacheVersionedValue}}
> already carries \{{cacheId}}, so the cache id travelled twice.
> *Fix:* the key moves into the value as the new \{{KeyedVersionedValue}}, and the field becomes a single
> collection. The response is \{{4 + 6N}} bytes shorter.
> h3. An array companion for an ordered set
> {\{TxLocksRequest#txKeys}} and \{{TxLocksResponse#txKeys}} kept a \{{Set}} plus an array to write it, even
> though \{{writeCollection}} and \{{writeObjectArray}} emit identical bytes.
> *Fix:* the companions are gone. The runtime type stays \{{HashSet}}, so the sender-side dedup is
> preserved and the wire format does not move.
> h3. A hand-written zip of two parallel fields
> {\{GridNearGetRequest#keyMap}} was assembled by pairing \{{keys}} and \{{readersFlags}} by index, and taken
> apart again on the way out.
> *Fix:* the map stays where the code reads better, and the conversion moves into
> {\{@Marshalled(keys = ..., values = ...)}}.
> h3. A map copied entry by entry into an empty one
> {\{GridDhtTxPrepareRequest#owned}} was drained into \{{GridNearTxRemote}} with \{{putAll}}, although the
> request is done with it by then.
> *Fix:* \{{ownedVersions}} takes the map over instead of copying it, which drops a \{{GridLeanMap}} and
> {\{N}} nodes per prepare.
> h3. Blob marshalling the codegen already expresses
> {\{LazyServiceConfigurationMessage#affKey}}, \{{GenericValueMessage#val}}, \{{QueryStartRequest#params}}
> and \{{QueryEntityMessage#dfltFieldValues}} each had a hand-written pair doing nothing but
> {\{U.marshal}}/\{{U.unmarshal}}.
> *Fix:* all four become \{{@Marshalled}}; the generated code matches the hand-written one call for call.
> {\{QueryEntityMessage}} keeps its \{{!F.isEmpty}} gate, moved into the constructor, because
> {\{QueryEntity}} initializes the defaults with an empty map while the generated gate is \{{!= null}}.
> h3. Collection marshalling in an atomic request
> {\{GridNearAtomicFullUpdateRequest#entryProcessors}} was marshalled by hand, so the request carried both
> the objects and their bytes until it was collected.
> *Fix:* the field becomes \{{@Marshalled}}, retyped to \{{List<Object>}} because the generated code calls
> {\{add(Object)}}. Two points worth reviewing:
> * the \{{operation() == TRANSFORM}} gates stay on \{{invokeArgs}} in all three methods. Dropping them only
> in \{{marshal}} would fill \{{invokeArgsBytes}} without calling \{{deployInvokeArguments}}, and a receiver
> with peer class loading on would get a \{{NoClassDefFoundError}} instead of today's silent degradation;
> * the \{{deploy()}} gate becomes \{{entryProcessors != null}}, because the codegen nulls the companion
> after restoring it, which makes the old \{{entryProcessorsBytes == null}} wording false in meaning.
> h3. A job payload unmarshalled with the wrong class loader
> {\{GridJobExecuteRequest}} kept two hand-written methods because its five payload fields are user classes
> that only the deployment class loader can read, while \{{GridIoManager#unmarshalPayload}} would hand them
> the configuration one.
> *Fix:* the fields become \{{@Marshalled}} and the class becomes a \{{DeferredUnmarshalMessage}}, the marker
> that exists for this case. \{{GridJobProcessor}} unmarshals the request itself, with the same loader as
> before. A job of a continuous task also stops sending its siblings: the receiver discarded them, and
> {\{GridJobSessionImpl#getJobSiblings}} requests them from the task node when they are missing.
> h3. Wire companions standing in for a conditional field
> Covers IGNITE-28922, to be closed as a duplicate.
> {\{CacheContinuousQueryEntry}} held \{{keyWire}}, \{{newValWire}} and \{{oldValWire}} purely so that
> {\{marshal()}} could hide the data of a filtered entry at serialization time.
> *Fix:* the three pairs collapse and \{{key}}, \{{newVal}} and \{{oldVal}} go back to plain \{{@Order}}. The
> invariant becomes structural instead: a filtered entry gives its data up at
> {\{CacheContinuousQueryEventBuffer#processEntry}}, the only door into the buffer, so nothing downstream
> has to remember it. Two copies this makes redundant are removed, and a filtered \{{EXPIRED}} entry with
> {\{updateCntr == -1}} is no longer sent at all, since \{{handleEvent}} already discarded it on arrival.
> h3. Cost
> The wire format is unchanged except where a section states a reduction, and no path allocates more than
> before, with one exception worth naming: a filtered entry taking the out-of-range counter path in the
> continuous query buffer now costs one object, where the data used to be masked at serialization time
> instead.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)