Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Sly parser initialization optimizations #16

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
Merged
merged 9 commits into from
May 12, 2025
Merged

Sly parser initialization optimizations #16

merged 9 commits into from
May 12, 2025

Conversation

ea-rus
Copy link
Collaborator

@ea-rus ea-rus commented Apr 30, 2025 •
edited
Loading

Applied code-optimization methods:

  • some dicts replaced by sets to speed up insert record if not exist
  • splitted caches to speed up search in smaller dict
  • circle with 'el == searched' replaced to prepared dict: ref.get(el)

Using cache for LRTable variables (the longest operation to calculate):

  • store them in json in pip package
  • in runtime it uses cache if exists, otherwise calculate as usual

To gen cache on client:

from mindsdb_sql_parser.parser import MindsDBParser
MindsDBParser.build_to_file()

!!! It requires write acces to pip module folder

Improvements, time to initialize:

  • 2.1 before optimizations
  • 0.7 after
  • 0.05 after aplying cache

@entelligence-ai-pr-reviews Entelligence AI PR Reviews
Copy link

Review Summary

Skipped posting 1 drafted comments based on your review threshold. Feel free to update them here.

Draft Comments
sly/yacc.py:982-986 
Memory leak in `lr_parse_table` method. The `self.grammar` reference is cleared after use, but `self.lr_productions` still holds a reference to `grammar.Productions`, preventing garbage collection.

Scores:

  • Production Impact: 4
  • Fix Specificity: 5
  • Urgency Impact: 4
  • Total Score: 13

Reason for filtering: The comment identifies a legitimate memory leak issue with a clear fix

Analysis: This comment identifies a memory leak where self.lr_productions maintains a reference to grammar.Productions preventing garbage collection. The fix is specific and directly applicable by adding 'self.lr_productions = None'. Memory leaks can accumulate over time and cause significant production issues, especially in long-running processes. The fix is straightforward and should be implemented soon to prevent resource exhaustion.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 30, 2025 •
edited
Loading

Coverage

Coverage Report
FileStmtsMissCoverMissing
mindsdb_sql_parser
   __about__.py10100%1–10
   __init__.py1162182%43, 47, 93, 110, 134–153, 160–161
   lexer.py2762192%362, 364, 366, 378, 380, 382, 388–406
   logger.py19479%14, 17, 23, 26
   parser.py10712897%120, 124, 263, 288, 385, 571, 588, 612–613, 821, 875, 952, 1043, 1096, 1106, 1145–1146, 1175, 1186, 1269, 1345, 1384, 1420, 1946, 1954, 2007–2010
   utils.py46491%73–79
mindsdb_sql_parser/ast
   base.py36586%13, 28, 31, 46, 51
   create.py801285%23–31, 92–97
   drop.py52296%10, 13
   insert.py63494%39–41, 46
   show.py48198%18
   update.py53591%40–42, 75–76
mindsdb_sql_parser/ast/mindsdb
   knowledge_base.py46198%79
mindsdb_sql_parser/ast/select
   case.py38295%19, 22
   constant.py36197%23
   data.py11464%10–12, 15, 19
   identifier.py71889%47, 91–96, 103
   native_query.py13192%25
   operation.py139497%56, 65, 177, 201
   parameter.py11191%10
   select.py109397%161–166
   star.py12283%8–9
TOTAL324014496% 

Tests Skipped Failures Errors Time
286 0 💤 0 ❌ 0 🔥 12.445s ⏱️

@ea-rus ea-rus requested a review from ZoranPandovski April 30, 2025 13:47
@ea-rus ea-rus merged commit 57b0aff into main May 12, 2025
9 checks passed
@ea-rus ea-rus deleted the sly-optimize branch May 12, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZoranPandovski ZoranPandovski ZoranPandovski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ea-rus @ZoranPandovski
Morty Proxy This is a proxified and sanitized view of the page, visit original site.