Skip to content

workaround for map declaration limit (ΛEnumTypes) #641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 28, 2022

Conversation

steiler
Copy link
Contributor

@steiler steiler commented Apr 13, 2022

As described in #640, When working with very large yang models like for instance Nokia SROS (https://github.com/nokia/7x50_YangModels/tree/master/latest_sros_22.2) the resulting ΛEnumTypes map exceeds certain golang limits.
As described in golang/go#33437 (comment) maps of too large size cannot be defined declaratively.
This PR is to implement the workaround mentioned in the above referenced issue. The ΛEnumTypes map is split into a Key (ΛEnumTypesKeys) and a Value (ΛEnumTypesValues) Slice and composed to the ΛEnumTypes map in the init() fuction. Which works around the declerative issue.

I am not sure about possible performance constraints or any other impact of this workaround. If that would be a problem, I can think of a more elaborate approach where this workaround is just applied when the map size is too large, otherwise the regular declarative approach is used.

@google-cla
Copy link

google-cla bot commented Apr 13, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

Copy link
Member

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestion.

In these really huge generated modules, is the code actually usable - i.e., is the binary size, memory footprint, and IDE features like autocomplete's performance acceptable? Part of me says if you're getting to extremely large generated modules, then these issues will be more of a problem than we've seen with relatively modest sized modules, such that something like the module splits that @wenovus and @DanG100 were looking at before would be advantageous. However, this is a longer term workaround.

In the meantime, I added a comment here about how the split is done. I think it'd be useful to look at this, and then subsequently whether there are performance regressions of doing this that might impact existing users.

ygen/gogen.go Outdated
@@ -418,6 +418,9 @@ var (

func init() {
var err error
for i:=0; i < len(ΛEnumTypesKeys); i++{
Copy link
Member

Choose a reason for hiding this comment

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

Why not just move creating the same type as below into init? I'm not really a fan of splitting this into keys and data, since there is now some requirement for ordering to be the same between the two, which seems like a hidden assumption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My take was, that it needed to be slices rather then a map and this was the proposed workaround in the referred thread.
However I've tested adding the map declaration into init(). Works fine so I'll try to change as proposed.

@steiler
Copy link
Contributor Author

steiler commented Apr 14, 2022

I did now push the ΛEnumTypes population into a function which is being called on init().
Are you guys fine with that approach?

@steiler
Copy link
Contributor Author

steiler commented Apr 14, 2022

In these really huge generated modules, is the code actually usable - i.e., is the binary size, memory footprint, and IDE features like autocomplete's performance acceptable? Part of me says if you're getting to extremely large generated modules, then these issues will be more of a problem than we've seen with relatively modest sized modules, such that something like the module splits that @wenovus and @DanG100 were looking at before would be advantageous. However, this is a longer term workaround.

Well lets be honest a go file of over 100MB that's not going to auto-complete like a charm. But for diffing and validation etc. that code works perfectly fine.

@steiler
Copy link
Contributor Author

steiler commented Apr 20, 2022

Finally also fixed all the tests and merged latest master.

@coveralls
Copy link

coveralls commented Apr 20, 2022

Coverage Status

Coverage remained the same at 90.409% when pulling f303310 on steiler:enumlimits into 231faad on openconfig:master.

@wenovus
Copy link
Contributor

wenovus commented Apr 20, 2022

Looks good @steiler, do you have the numbers on how big the binary size increase is (The .a files for exampleoc and friends).

@steiler
Copy link
Contributor Author

steiler commented Apr 25, 2022

steiler@vbox:~/projects/ygot$ \
> git restore exampleoc/*.go ; \
> git checkout master; \
> git log -1 ; \
> cd exampleoc ; \
> go generate &>/dev/null ; \
> cd - ; go tool pack c /tmp/master.a exampleoc/*.go ; \
> git restore exampleoc/*.go ; \
> git checkout enumlimits ; \
> git log -1 ; \
> cd exampleoc ; \
> go generate &>/dev/null ; \
> cd - ; go tool pack c /tmp/enumlimits.a exampleoc/*.go ; \
> ls -la /tmp/*.a

Switched to branch 'master'
Your branch is up to date with 'origin/master'.
commit f9339e19b5cf34c35a94d11f60f48c596c9dde29 (HEAD -> master, origin/master, origin/HEAD)
Author: Rob Shakir <[email protected]>
Date:   Fri Apr 22 10:31:18 2022 -0700

    Remove log that is always generated for annotation fields. (#652)

     * (M) ytypes/container.go
      - Since schemas that are generated with annotations will always
        generate the log that says that unmarshal into them is unsupported
        the log message produced is extremely low value to a user. To this
        end - just remove it.
/home/steiler/projects/ygot
Switched to branch 'enumlimits'
Your branch is up to date with 'steiler/enumlimits'.
commit fa5b3a9144894798182bb22a43d4f811dc1e10fd (HEAD -> enumlimits, steiler/enumlimits)
Merge: 5fbee45 c15884d
Author: steiler <[email protected]>
Date:   Wed Apr 20 16:35:46 2022 +0200

    Merge branch 'master' of https://github.com/openconfig/ygot into enumlimits
/home/steiler/projects/ygot
-rw-rw-r-- 1 steiler steiler 25863424 Apr 25 11:07 /tmp/enumlimits.a
-rw-rw-r-- 1 steiler steiler 25862162 Apr 25 11:06 /tmp/master.a

So it's
25863424 byte - /tmp/enumlimits.a
vs.
25862162 byte - /tmp/master.a

@wenovus
Copy link
Contributor

wenovus commented Apr 25, 2022

@robshakir Does this look good to you?

@wenovus wenovus merged commit 056010a into openconfig:master Apr 28, 2022
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.

4 participants