[
https://issues.apache.org/jira/browse/IGNITE-28935?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Anton Vinogradov updated IGNITE-28935:
--------------------------------------
Labels: IEP-132 wire-format (was: IEP-132)
> Simplify how the marshaller is chosen for message fields
> --------------------------------------------------------
>
> Key: IGNITE-28935
> URL: https://issues.apache.org/jira/browse/IGNITE-28935
> Project: Ignite
> Issue Type: Task
> Components: messaging
> Reporter: Anton Vinogradov
> Assignee: Anton Vinogradov
> Priority: Major
> Labels: IEP-132, wire-format
> Fix For: 2.19
>
>
> h3. Goal
> Make it clear which marshaller is used for each message field. Today this is
> set in 60 places. It should be set in none.
> h3. Why
> An object field of a message becomes bytes either by {{JdkMarshaller}} or by
> {{BinaryMarshaller}}. The marshaller is chosen per message class: the
> {{@UseBinaryMarshaller}} annotation plus the call in {{CoreMessagesProvider}}.
> Numbers from master, counted in the generated companions:
> * core registers 297 messages: 58 binary, 239 jdk;
> * 195 {{*Marshaller}} companions are generated, but only 30 keep a
> {{Marshaller}};
> * the choice changes the result for 24 classes only;
> * there are 60 annotations. Only 13 of them reach a companion that takes a
> {{Marshaller}} at all, and of those
> only 11 change any byte. The other 47 are noise.
> How the numbers are counted. The annotation decides one thing only: which
> {{Marshaller}} instance is passed to the
> constructor of the generated companion. The generator emits a constructor
> with a {{Marshaller}} only when the class
> has something to marshal ({{marshallable || hasMarshalled}}); for everything
> else the companion has a no-arg
> constructor and {{register(...)}} drops the marshaller it was given
> ({{ctor.getParameterCount() == 0 ? newInstance() : newInstance(marsh)}}). So:
> {code}
> git grep -l "@UseBinaryMarshaller" master -- '*.java' | wc -l # 60
> (58 core, 2 calcite)
> grep -rl "private final Marshaller marshaller" \
> modules/core/target/generated-sources/codegen | wc -l # 30
> of 195 companions
> {code}
> The intersection of the two sets is 13. Within those 13:
> * generated code calls {{U.marshal(marshaller, ...)}} - 8:
> {{BinaryMetadataVersionInfo}}, {{GridCacheQueryRequest}},
> {{GridCacheQueryResponse}}, {{GridEventStorageMessage}},
> {{GridJobSiblingsResponse}},
> {{GridNearAtomicSingleUpdateInvokeRequest}}, {{GridTaskResultResponse}},
> {{IgniteTxEntry}};
> * only the {{msg.marshal(marshaller)}} hook, and the hand-written body does
> use the marshaller - 3:
> {{GridDhtAtomicUpdateRequest}} (dead code, removed by IGNITE-28936),
> {{GridNearAtomicFullUpdateRequest}}, {{GridTopicMessage}};
> * only the hook, and the marshaller is not used at all - 2:
> {{GridCacheEntryInfo}} (TTL recalculation),
> {{CacheContinuousQueryEntry}} (data reset).
> The annotation is meant to say "this message carries user classes", but that
> is not what it does. Here is the
> complete list of fields where user data travels with jdk today:
> || class || fields ||
> | {{TcpDiscoveryNode}} | {{consistentId}}, {{attrs}} |
> | {{TcpDiscoveryNodeAddFinishedMessage}} | {{clientNodeAttrs}} |
> | {{StoredCacheData}} | {{ccfg}}, {{qryEntities}} |
> | {{DynamicCacheChangeRequest}} | {{startCfg}}, {{nearCacheCfg}}, {{schema}} |
> | {{ChangeGlobalStateMessage}} | {{baselineTopology}} |
> | {{QueryEntityMessage}}, {{QueryEntityExMessage}} | {{dfltFieldValues}} |
> | {{LazyServiceConfigurationMessage}} | {{affKey}} |
> | {{PluginsDataBagItem}} | {{data}} |
> | {{SerializableDataBagItemWrapper}} | {{data}} |
> | {{PartitionHashRecord}} | {{consistentId}}, {{updateCntr}} |
> | {{TransactionsHashRecord}} | {{locConsistentId}}, {{rmtConsistentId}} |
> | {{ErrorMessage}} | {{err}} - a {{Throwable}}, user classes included |
> The same jdk marshaller also carries user classes to disk, outside messages:
> {{cache_data.dat}}, the metastore and
> snapshots keep a {{CacheConfiguration}} with its {{NodeFilter}},
> {{ExpiryPolicyFactory}}, {{CacheStoreFactory}},
> {{Interceptor}} and {{AffinityFunction}} inside.
> The rule that actually holds is about *class registration*, not about user
> data. Before binary can write a type it must map class name to type id for
> the whole cluster: {{MarshallerContextImpl#registerClassName}} ->
> {{proposeMapping}} -> {{fut.get()}}, which waits for discovery. Ignite
> classes avoid that call because 2276 names are pre-accepted from
> {{META-INF/classnames.properties}} - but that list is not closed under
> nesting. Marshalling a plain {{CacheConfiguration}} needs 9 types, and two of
> them are missing from the list:
> * {{javax.cache.configuration.FactoryBuilder$SingletonFactory}}
> * {{javax.cache.expiry.EternalExpiryPolicy}}
> They come from the default expiry policy, with no user class involved. So a
> discovery message that carries a cache configuration would trigger a
> cluster-wide registration from inside the discovery thread.
> {{failIfUnregistered = true}} does not help: it turns the deadlock into an
> {{UnregisteredClassException}}, and it fires for internal types as well.
> So "discovery uses jdk" is not a property of the binary format. It is a
> consequence of where the marshalling happens and of what is pre-registered.
> The call site knows both; the message class knows neither.
> Binding the marshaller to the class also produces three defects:
> # The annotation is not {{@Inherited}}, but the generator builds a companion
> from all fields, including inherited ones. Nine parent/child pairs are
> registered in different groups. Checked with an experiment: add a blob field
> to {{GridDistributedTxPrepareRequest}} (binary), and the generated
> {{GridNearTxPrepareRequestMarshaller}} (jdk) marshals the same field with
> jdk. No such field exists today, so this is a trap, not a live bug.
> # A nested message overrides the outer one, because the marshaller is taken
> by {{directType}}. {{ErrorMessage}} (jdk) travels inside binary messages, and
> {{BinaryMetadataVersionInfo}} (binary) travels inside the jdk discovery data
> bag. The same {{BinaryMetadata}} in {{MetadataUpdateProposedMessage}} uses
> jdk.
> # {{MarshallableMessage}} means two things at once: "call me before send" and
> "I need a marshaller". 8 of its 21 implementations use neither the marshaller
> nor the class loader.
> h3. Measurements
> Round trip marshal+unmarshal, 5000 iterations after 2000 warm-up ones, JDK
> 17, local marshaller context.
> Size in bytes / time in microseconds.
> || object || jdk || optimized || binary, type registered || binary, class
> name on the wire ||
> | node attributes, Map<String,Object> x50 | 2725 / 30.0 | 2426 / 11.3 | 2423
> / 5.2 | 2423 / 5.0 |
> | CacheConfiguration | 4539 / 39.4 | 1011 / 15.8 | 857 / 24.0 | 1382 / 160.2 |
> | QueryEntity | 1144 / 9.9 | 345 / 2.9 | 332 / 2.4 | 454 / 35.7 |
> | IgniteCheckedException | 709 / 6.7 | 710 / 6.4 | 715 / 7.4 | 715 / 9.2 |
> What this says:
> * binary is the fastest and the smallest option *when the types are already
> registered*;
> * the exception is the same everywhere, because binary hands every
> {{Throwable}} to {{OptimizedMarshaller}}:
> {{BinaryUtils#isCustomJavaSerialization}} walks the whole class hierarchy,
> so any {{Externalizable}} class or a
> class with {{writeObject}}/{{readObject}} takes that path;
> * writing the class name instead of a type id costs both size and a lot of
> time - the descriptor of an
> unregistered type is not cached and is rebuilt by reflection on every call.
> h3. Plan
> Three subtasks change the bytes on the wire. They should be done *first*,
> while 2.19 is not released yet. After the release the same changes would
> break rolling upgrade and would need a feature flag plus support for both
> formats.
> *Do first - these change the wire format:*
> # IGNITE-28936 - *remove dead marshalling in {{GridDhtAtomicUpdateRequest}}.*
> The flag is always {{false}}, so this code never runs. Result: 4 fields less
> on the wire.
> # IGNITE-28939 - *turn blob fields of internal types into normal message
> fields* ({{BinaryMetadata}}, {{BaselineTopology}}, job siblings, query
> entities). Result: 24 blob fields become about 19, and the {{BinaryMetadata}}
> mismatch is gone.
> # IGNITE-28940 - *pass the marshaller from the call site.* This is the main
> change. Communication passes binary, discovery passes jdk. Four classes
> change their format on purpose. Result: no annotations left, and a wrong
> marshaller becomes impossible.
> *Can be done any time - no wire format change:*
> # IGNITE-28937 - *one registration form in {{CoreMessagesProvider}}.*
> {{withSchema}} and {{withNoSchema}} are the same code now. Result: the
> marshaller is written in one place, generated code does not change.
> # IGNITE-28938 - *split {{MarshallableMessage}}.* 8 of 21 classes only need a
> callback. Result: they stop getting a marshaller. Small task: if it is done
> before IGNITE-28940, that one becomes smaller.
> # IGNITE-28941 - *add a test for the discovery path.* Result: the rule is
> checked by CI, not kept in mind.
> # IGNITE-28942 - *research: can binary replace jdk entirely.* No production
> change expected; see the section below.
> h3. Considered and not planned: dropping JdkMarshaller
> Binary can be self-contained. When a type is not registered it writes
> {{UNREGISTERED_TYPE_ID}} plus the class name
> ({{BinaryWriterExImpl#preWrite}}), and with a full footer the schema travels
> inside the object. Checked: an object written by one context is read by a
> completely empty one, cycles included, 84 bytes against 121 for jdk. With the
> default compact footer the same object fails to read - "Cannot find metadata
> for object with compact footer" - because the field layout still comes from
> the metadata that discovery distributes.
> So the format can replace jdk, but the move is not worth it today:
> * {{compactFooter}} is a node-wide setting, a per-call full footer mode would
> have to be added;
> * the unregistered path is 6.7x slower until descriptors are cached;
> * on-disk formats (cache configurations, metastore, snapshots, dumps) are
> written with jdk and read by offline tools, so this becomes a data migration;
> * {{OptimizedMarshaller}} stays anyway - binary delegates every
> {{Externalizable}} type to it.
> That is three implementations turning into two, at the price of a new format
> mode and a migration. See IGNITE-28942 for the details and for the open
> questions.
> h3. On-disk data
> The plan does not touch anything that is already stored. Disk paths do not go
> through {{MessageMarshalling}}:
> cache configurations and the metastore are written with jdk, binary metadata
> through its own {{Externalizable}}
> methods, and the direct message protocol ({{MessageSerializer}}) is used for
> the network only - never for files.
> Wire companions in classes that are both {{Serializable}} and {{Message}} are
> {{transient}}, so the two formats do
> not interfere. IGNITE-28939 is the only subtask that touches classes living
> on disk; it carries the rules that keep
> the stored format unchanged. No migration is required by any subtask.
> h3. Expected result
> || || now || after ||
> | places that choose the marshaller | 60 annotations | 0 |
> | companions that keep a {{Marshaller}} | 30 | about 12 |
> | classes that get a marshaller and never use it | 6 | 0 |
> | blob fields of internal types | 24 | about 19 |
> | wrong marshaller from inheritance or nesting | possible | not possible |
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
