Skip to content

protodoc: Pre-parse/validate the protodoc manifest#21559

Merged
phlax merged 9 commits intoenvoyproxy:mainfrom
phlax:tooling-protodoc-manifest
Jun 9, 2022
Merged

protodoc: Pre-parse/validate the protodoc manifest#21559
phlax merged 9 commits intoenvoyproxy:mainfrom
phlax:tooling-protodoc-manifest

Conversation

@phlax
Copy link
Member

@phlax phlax commented Jun 2, 2022

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message:
Additional Description:

Currently the protodoc_manifest which contains api examples, is validated in every protodoc process

This moves the validation to a separate job and builds a json artefact from the validated data

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax marked this pull request as draft June 2, 2022 18:46
@phlax phlax force-pushed the tooling-protodoc-manifest branch 2 times, most recently from 1c023b9 to 610fc4a Compare June 2, 2022 19:27
@phlax
Copy link
Member Author

phlax commented Jun 2, 2022

/docs

@repokitteh-read-only
Copy link

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/21559/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #21559 (comment) was created by @phlax.

see: more, trace.

@phlax phlax force-pushed the tooling-protodoc-manifest branch from 610fc4a to 8dcacde Compare June 2, 2022 19:41
@phlax phlax changed the title [WIP] protodoc: Pre-validate the protodoc manifest protodoc: Pre-validate the protodoc manifest Jun 2, 2022
@phlax phlax force-pushed the tooling-protodoc-manifest branch from 8dcacde to c23113e Compare June 2, 2022 19:44
@phlax phlax marked this pull request as ready for review June 2, 2022 19:44
@phlax phlax force-pushed the tooling-protodoc-manifest branch from c23113e to 3c44727 Compare June 2, 2022 19:55
@phlax phlax changed the title protodoc: Pre-validate the protodoc manifest protodoc: Pre-parse/validate the protodoc manifest Jun 2, 2022
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the tooling-protodoc-manifest branch from 3c44727 to 3b91640 Compare June 2, 2022 20:21
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Can you indicate the performance improvement on these optimizations? Seems reasonable, yet we could arbtirarily preprocess things and it would be good to focus on the heavy hitters.

@phlax
Copy link
Member Author

phlax commented Jun 4, 2022

thisi s probably the most important optimization that i found (none were huge)

i can get some timings tomorrow - but the point is - moving the validation out of protodoc means that the protodoc environment is leaner (no runfiles, config_validation, etc)

the validation happening here is also pointless - it is validating the same thing repeatedly

even if it doesnt give a massive increase getttng this one out of the way is an important step to the next PR - which mangles all required data for protodoc into a single json object - see here https://github.com/envoyproxy/envoy/pull/21561/files#diff-1e2d0f2d31a35d12f876114dd98bde49a7559427f00329f9b9c33aa8d494d569R86

phlax added 2 commits June 4, 2022 12:11
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Member Author

phlax commented Jun 4, 2022

With the following diff:

diff --git a/tools/api_proto_plugin/traverse.py b/tools/api_proto_plugin/traverse.py
index a27813ffa5..6bf1aee991 100644
--- a/tools/api_proto_plugin/traverse.py
+++ b/tools/api_proto_plugin/traverse.py
@@ -1,7 +1,12 @@
 """FileDescriptorProto traversal for api_proto_plugin framework."""
 
+import logging
+import time
+
 from tools.api_proto_plugin import type_context
 
+logger = logging.getLogger(__name__)
+
 
 def traverse_service(type_context, service_proto, visitor):
     """Traverse a service definition.
@@ -74,6 +79,7 @@ def traverse_file(file_proto, visitor):
     Returns:
         Plugin specific output.
     """
+    start = time.time()
     source_code_info = type_context.SourceCodeInfo(file_proto.name, file_proto.source_code_info)
     package_type_context = type_context.TypeContext(source_code_info, file_proto.package)
     services = [
@@ -91,4 +97,6 @@ def traverse_file(file_proto, visitor):
             package_type_context.extend_enum(index, enum.name, enum.options.deprecated), enum,
             visitor) for index, enum in enumerate(file_proto.enum_type)
     ]
-    return visitor.visit_file(file_proto, package_type_context, services, msgs, enums)
+    result = visitor.visit_file(file_proto, package_type_context, services, msgs, enums)
+    logger.warning(f"VISITED {file_proto.name}: {time.time() - start}s")
+    return result

and ommitting logs for all but the worst offender...

before

VISITED envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto: 10.386241436004639s

after

VISITED envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto: 0.43071722984313965s

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Member Author

phlax commented Jun 4, 2022

in terms of protodoc itself this opti is huge - but as discussed offline very little of the time generating the api docs is actually spent in protodoc itself so the overall effect is not massive

phlax added 2 commits June 4, 2022 14:10
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
phlax added 2 commits June 8, 2022 02:17
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Member Author

phlax commented Jun 8, 2022

this can be sped up considerably by making use of the v3_proto_set added in #21579 - i have opened #21607 to work on that

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Member Author

phlax commented Jun 8, 2022

@htuch it would be great to land this one

currently i am rebasing my dev branches on this and the no-aspect branch as between them they make building docs/ running tests etc bearable

a couple of my WIP PRs are waiting on this one, and i also have further optimizations that i know of to this script that i am waiting to add

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Feel free to merge when main is unlocked.

@phlax phlax merged commit 10d3aab into envoyproxy:main Jun 9, 2022
tyxia pushed a commit to tyxia/envoy that referenced this pull request Jun 14, 2022
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Amila-Rukshan pushed a commit to Amila-Rukshan/envoy that referenced this pull request Jun 28, 2022
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>
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.

2 participants