Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Load faster versions of safe loaders as well #47422

Closed
wants to merge 6 commits into from
Closed

Load faster versions of safe loaders as well #47422

wants to merge 6 commits into from

Conversation

jacksontj
Copy link
Contributor

The C versions of the loader/dumper are 5-10x faster than the python
versions. We already do this for the "unsafe" ones, this diff simply
adds it for the "safe" ones.

@jacksontj
Copy link
Contributor Author

Seems like CI doesn't like me:

  • The py3 failures seem unrelated,
  • jenkins/PR/salt-pr-rs-cent7-n also seems unrelated
  • jenkins/PR/salt-pr-linode-ubuntu14-n unable to reproduce those errors locally

@jacksontj
Load faster versions of safe loaders as well
2eacf76 
The C versions of the loader/dumper are 5-10x faster than the python
versions. We already do this for the "unsafe" ones, this diff simply
adds it for the "safe" ones.
@jacksontj
Copy link
Contributor Author

Rebased on current master

@jacksontj
Copy link
Contributor Author

@cachedout want me to rebase and try again?

@jacksontj
Copy link
Contributor Author

@rallytime any ideas/details why this one is failing?

@rallytime
Copy link
Contributor

@jacksontj I wasn't able to check these results before they timed out, so I have restarted them.

terminalmage
terminalmage approved these changes Jun 1, 2018
View reviewed changes
Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

This makes sense. @rallytime any objections with backporting this into 2018.3?

@rallytime rallytime added the bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch label Jun 1, 2018
@rallytime
Copy link
Contributor

@terminalmage Nope, no objections. Labeled for backport.

@rallytime
Copy link
Contributor

There are a couple of related test failures on this one, however:

  • unit.utils.test_yamlloader.YamlLoaderTestCase.test_yaml_with_colon_in_inline_dict
  • unit.templates.test_jinja.TestCustomExtensions.test_load_yaml

@jacksontj Can you take a look?

@terminalmage
Copy link
Contributor

I don't know that we're going to be able to merge this. Those failures are happening because we have subclassed the SafeLoader and have made modifications on top of it. Those changes don't work when the CSafeLoader takes the place of the SafeLoader.

@terminalmage terminalmage added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jun 3, 2018
@jacksontj
Copy link
Contributor Author

@terminalmage got a way to reproduce the error locally? I'd gladly take some time to dig into it, I'm just unable to repro these failures locally (so maybe some yaml version difference or something).

@terminalmage
Copy link
Contributor

@jacksontj Sure. The failure is a test I added to confirm a fix that was made to handle colons in inline python unicode literals, see #47104. And you can reproduce the failure like so:

>>> import salt.utils.yaml
>>> salt.utils.yaml.safe_load('''\
... foo: {u'url': u'https://foo.com'}
... ''')
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File "/testing/salt/utils/yamlloader.py", line 226, in safe_load
    return yaml.load(stream, Loader=Loader)
  File "/usr/lib64/python2.7/site-packages/yaml/__init__.py", line 71, in load
    return loader.get_single_data()
  File "/usr/lib64/python2.7/site-packages/yaml/constructor.py", line 37, in get_single_data
    node = self.get_single_node()
  File "_yaml.pyx", line 707, in _yaml.CParser.get_single_node (ext/_yaml.c:8308)
  File "_yaml.pyx", line 725, in _yaml.CParser._compose_document (ext/_yaml.c:8581)
  File "_yaml.pyx", line 776, in _yaml.CParser._compose_node (ext/_yaml.c:9306)
  File "_yaml.pyx", line 890, in _yaml.CParser._compose_mapping_node (ext/_yaml.c:10838)
  File "_yaml.pyx", line 776, in _yaml.CParser._compose_node (ext/_yaml.c:9306)
  File "_yaml.pyx", line 890, in _yaml.CParser._compose_mapping_node (ext/_yaml.c:10838)
  File "_yaml.pyx", line 732, in _yaml.CParser._compose_node (ext/_yaml.c:8685)
  File "_yaml.pyx", line 905, in _yaml.CParser._parse_next_event (ext/_yaml.c:11045)
yaml.scanner.ScannerError: while scanning a plain scalar
  in "<byte string>", line 1, column 15
found unexpected ':'
  in "<byte string>", line 1, column 22

The reason we needed that fix is because one of the ways people use Salt is using the json Jinja filter to dump a data structure to inline JSON with an SLS file. Since PyYAML is able to load inline Python data structures, loading a string like the one in the example above will result in a ScannerError.

@jacksontj
Copy link
Contributor Author

jacksontj commented Jun 18, 2018 •
edited

That sounds like we should have made that behavior (loading raw prints of python objects) as a special loader, since that is invalid yaml :/ How tied are we to that behavior? If very, then maybe I can add a "high-perf" loader option that doesn't have that magic behavior?

cc: @terminalmage

@jacksontj jacksontj mentioned this pull request Jun 18, 2018
Closed
jacksontj added a commit to jacksontj/salt that referenced this pull request Jun 19, 2018
@jacksontj
Add cyaml renderer
0eebd66 
The default salt yaml renderer overrides the default python yaml
library, which is quite a bit slower. Unfortunately over time a few
non-yaml things have been added into the default salt yaml loader
(example:
saltstack#47422 (comment)). As
such it makes it difficult to move to a C-backed implementation without
making our own.

This simply adds a `cyaml` renderer as an option to salt. With this
users can decide to switch to a more strict yaml parser (no python
prints) which is ~5-12x faster. Eventually we may decide to make this
the default, but by adding this as a renderer option there is no rush to
do so.
@jacksontj
Copy link
Contributor Author

Superseded by #48194

@jacksontj jacksontj closed this Jun 19, 2018
@rallytime rallytime removed the bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch label Jun 19, 2018
@jacksontj jacksontj reopened this Jun 21, 2018
@jacksontj
Copy link
Contributor Author

Re-opening. Copy-paste from comment in the other PR:

After talking with @terminalmage more it looks like we can probably go back to the approach in #47422 as the one feature we are trying to preserve is actually a bug :)

@rallytime
Copy link
Contributor

@jacksontj and @terminalmage I think this has been replaced with #48309, yes?

@rallytime
Copy link
Contributor

I'm fairly certain this was replaced by #48309 so I will close this.

If I have closed this too quickly, let me know I will re-open. :)

Note, there is a merge conflict in salt.utils.yamlloader.py that will need to be fixed first.

@rallytime rallytime closed this Jul 13, 2018
@jacksontj jacksontj deleted the yaml_perf branch July 27, 2018 14:36
@jacksontj
Copy link
Contributor Author

Your assessment looks accurate :) Thanks!

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

@terminalmage terminalmage terminalmage approved these changes

Assignees
No one assigned
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jacksontj @rallytime @terminalmage