[
https://issues.apache.org/jira/browse/IGNITE-28935?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Anton Vinogradov updated IGNITE-28935:
--------------------------------------
Description:
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, and only 13 of them matter. The other 47 do nothing.
The annotation is meant to say "this message carries user classes", but that is
not what it does. User data goes with jdk in {{TcpDiscoveryNode#attrs}},
{{LazyServiceConfigurationMessage#affKey}}, {{StoredCacheData#ccfg}} and
{{QueryEntityMessage#dfltFieldValues}}.
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. 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 |
was:
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, and only 13 of them matter. The other 47 do nothing.
The annotation is meant to say "this message carries user classes", but that is
not what it does. User data goes with jdk in {{TcpDiscoveryNode#attrs}},
{{LazyServiceConfigurationMessage#affKey}}, {{StoredCacheData#ccfg}} and
{{QueryEntityMessage#dfltFieldValues}}.
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.
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 the evaluation subtask for the details.
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 |
> 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
> 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, and only 13 of them matter. The other 47 do
> nothing.
> The annotation is meant to say "this message carries user classes", but that
> is not what it does. User data goes with jdk in {{TcpDiscoveryNode#attrs}},
> {{LazyServiceConfigurationMessage#affKey}}, {{StoredCacheData#ccfg}} and
> {{QueryEntityMessage#dfltFieldValues}}.
> 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. 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)
