Skip to content

Comments

feat: patch ProtoBuf to generate mutable structs#2174

Open
avik-pal wants to merge 2 commits intomainfrom
ap/patch_protobuf_to_emit_mutable_structs
Open

feat: patch ProtoBuf to generate mutable structs#2174
avik-pal wants to merge 2 commits intomainfrom
ap/patch_protobuf_to_emit_mutable_structs

Conversation

@avik-pal
Copy link
Collaborator

No description provided.

@gbaraldi
Copy link

So I took a much longer look and this won't help. The functions that are generated are just super unwieldy for the big structs like DebugOptions. They generate several mb of IR each. An actual fix would be to not use a static structure but move to some more dynamic handling of those protobufs by making them for example Dicts. I doubt we need the extreme runtime performance that specializing on a message gives you

@gbaraldi
Copy link

I did an experiment of turning photo into a separate sub package like ReactantCore. It takes 20 seconds to compile by itself 🥶

@avik-pal
Copy link
Collaborator Author

We can do #2175 + make the functionality available via another package (which must be manually loaded).

@avik-pal
Copy link
Collaborator Author

Converting to a dict is also probably pretty simple. We convert the structs to dicts and then forward the getproperty to the dict

field_types = [strip(fm.captures[2]) for fm in field_matches]

# Build default values dict for getproperty
defaults_entries = [" :$(fn) => $(get_default_for_type(ft))" for (fn, ft) in zip(field_names, field_types)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
defaults_entries = [" :$(fn) => $(get_default_for_type(ft))" for (fn, ft) in zip(field_names, field_types)]
defaults_entries = [
" :$(fn) => $(get_default_for_type(ft))" for
(fn, ft) in zip(field_names, field_types)
]

args = split_args(args_str)
if length(args) == num_fields
# Build keyword constructor call
kwargs = join(["$(fn)=$(strip(arg))" for (fn, arg) in zip(field_names, args)], ", ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
kwargs = join(["$(fn)=$(strip(arg))" for (fn, arg) in zip(field_names, args)], ", ")
kwargs = join(
[
"$(fn)=$(strip(arg))" for
(fn, arg) in zip(field_names, args)
],
", ",
)

@giordano
Copy link
Member

Bump. Reactant precompile time is still atrocious, which also badly affects CI here

  492950.5 ms  ✓ Reactant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants