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.

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

Recognize certain Junos AS Path regexes and convert them to VI construct #8770

Merged
merged 6 commits into from Aug 14, 2023

Conversation

samarthdhingra
Copy link
Contributor

@samarthdhingra samarthdhingra commented Aug 9, 2023

Performance optimization for some of Junos AS path regex patterns by converting them to existing VI construct AsSetsMatchingRanges . This bypasses converting these junos specific regular expression to java regular expression.

Following Junos AS Path regex patterns are included :

  • ".* N .*" : "AS Path contains N"
  • ".* N" or ".* N$" : "AS Path ends with N"
  • "N .*" or "^N .*" : "AS Path starts with N".
  • ".* [start-end] .*" OR ".* start-end .*" : "AS Path contains an ASN in range between start and end included"
  • "[start-end]" or "start-end" : "AS Path matches single ASN number in range between start and end included
  • "N": "AS Path matches single ASN number"

If one of the above patterns don't match, then fallback to the existing solution of converting to java regex using AsPathRegex

As part of testing using bf_session.init_snapshot_from_text(..) for a single device config with multiple range pattern regexes, snapshot initialization time decreased from ~155s to ~3s.

@batfish-bot
Copy link

This change is Reviewable

Copy link
Contributor

@anothermattbrown anothermattbrown left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @samarthdhingra)


projects/batfish/src/test/java/org/batfish/representation/juniper/AsPathMatchExprParserTest.java line 97 at r1 (raw file):

    @Test
    public void testAsPathMatchRegexFallback() {
        for (String regex : new String[]{"1234", "1234?", "1234{0,1}", "12{1,4} 34", "[123-125]", "123 (56 | 78)?", "()"}) {

this pattern would be good to optimize

Code quote:

"[123-125]",

@samarthdhingra
Copy link
Contributor Author

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @samarthdhingra)

projects/batfish/src/test/java/org/batfish/representation/juniper/AsPathMatchExprParserTest.java line 97 at r1 (raw file):

    @Test
    public void testAsPathMatchRegexFallback() {
        for (String regex : new String[]{"1234", "1234?", "1234{0,1}", "12{1,4} 34", "[123-125]", "123 (56 | 78)?", "()"}) {

this pattern would be good to optimize

Code quote:

"[123-125]",

Done.
Added following new patterns :

"N": "AS Path matches single ASN number"
"[start-end]" or "start-end" : "AS Path matches single ASN number in range between start and end included
".* start-end .*" : "AS Path contain ASN number in range between start and end included

Copy link
Contributor

@anothermattbrown anothermattbrown left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @samarthdhingra)

Copy link
Contributor

@anothermattbrown anothermattbrown left a comment

Choose a reason for hiding this comment

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

Have you measured how much this speeds up parsing configs with these patterns, and how much its slow down parsing configs without them?

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @samarthdhingra)

@samarthdhingra
Copy link
Contributor Author

samarthdhingra commented Aug 11, 2023

Have you measured how much this speeds up parsing configs with these patterns, and how much its slow down parsing configs without them?

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @samarthdhingra)

Depending on the as-path regex pattern, I observe most execution time improvement if the junos regex contains range pattern .* start-end .* or .* [start-end] .* (depending on the different between the ranges).

For the sample config tested using bf_session.init_snapshot_from_text(..) the speed up was from ~155s to ~3s

Copy link
Contributor

@anothermattbrown anothermattbrown left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @samarthdhingra)

Copy link
Contributor

@anothermattbrown anothermattbrown left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @samarthdhingra)

@dhalperi dhalperi enabled auto-merge (squash) August 14, 2023 17:13
@dhalperi dhalperi disabled auto-merge August 14, 2023 17:20
@dhalperi dhalperi enabled auto-merge (squash) August 14, 2023 17:21
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #8770 (6b49010) into master (15abe44) will increase coverage by 0.00%.
The diff coverage is 98.43%.

❗ Current head 6b49010 differs from pull request most recent head 6584618. Consider uploading reports for the commit 6584618 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8770   +/-   ##
=======================================
  Coverage   72.32%   72.32%           
=======================================
  Files        3288     3289    +1     
  Lines      168552   168612   +60     
  Branches    19755    19764    +9     
=======================================
+ Hits       121907   121956   +49     
- Misses      37552    37561    +9     
- Partials     9093     9095    +2     
Files Changed Coverage Δ
.../representation/juniper/AsPathMatchExprParser.java 98.27% <98.27%> (ø)
...g/batfish/representation/juniper/PsFromAsPath.java 66.66% <100.00%> (+2.38%) ⬆️
...fish/representation/juniper/PsFromAsPathGroup.java 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

auto-merge was automatically disabled August 14, 2023 20:25

Head branch was pushed to by a user without write access

@dhalperi dhalperi enabled auto-merge (squash) August 14, 2023 20:43
Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @samarthdhingra)

@dhalperi dhalperi merged commit 78fbd70 into batfish:master Aug 14, 2023
7 checks passed
@samarthdhingra samarthdhingra deleted the junos-aspath-regex branch August 14, 2023 21:06
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.

None yet

4 participants