Skip to content

Commit

Permalink
perf(hogql): Fix C++ parser leaks (#18022)
Browse files Browse the repository at this point in the history
  • Loading branch information
Twixes authored Oct 17, 2023
1 parent df282f6 commit 711f2b4
Show file tree
Hide file tree
Showing 6 changed files with 190 additions and 132 deletions.
269 changes: 142 additions & 127 deletions hogql_parser/parser.cpp

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion hogql_parser/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

setup(
name="hogql_parser",
version="0.1.7",
version="0.1.8",
url="https://github.com/PostHog/posthog/tree/master/hogql_parser",
author="PostHog Inc.",
author_email="[email protected]",
Expand Down
11 changes: 9 additions & 2 deletions posthog/hogql/test/_test_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,18 @@
from posthog.hogql.errors import HogQLException, SyntaxException
from posthog.hogql.parser import parse_expr, parse_order_expr, parse_select
from posthog.hogql.visitor import clear_locations
from posthog.test.base import BaseTest
from posthog.test.base import BaseTest, MemoryLeakTestMixin


def parser_test_factory(backend: Literal["python", "cpp"]):
class TestParser(BaseTest):
base_classes = (MemoryLeakTestMixin, BaseTest) if backend == "cpp" else (BaseTest,)

class TestParser(*base_classes):
MEMORY_INCREASE_PER_PARSE_LIMIT_B = 10_000
MEMORY_INCREASE_INCREMENTAL_FACTOR_LIMIT = 0.1
MEMORY_PRIMING_RUNS_N = 2
MEMORY_LEAK_CHECK_RUNS_N = 100

maxDiff = None

def _expr(self, expr: str, placeholders: Optional[Dict[str, ast.Expr]] = None) -> ast.Expr:
Expand Down
36 changes: 36 additions & 0 deletions posthog/test/base.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import datetime as dt
import inspect
import re
import resource
import threading
import uuid
from contextlib import contextmanager
Expand Down Expand Up @@ -194,6 +195,41 @@ def validate_basic_html(self, html_message, site_url, preheader=None):
self.assertIn(preheader, html_message) # type: ignore


class MemoryLeakTestMixin:
MEMORY_INCREASE_PER_PARSE_LIMIT_B: int
"""Parsing more than once can never increase memory by this much (on average)"""
MEMORY_INCREASE_INCREMENTAL_FACTOR_LIMIT: float
"""Parsing cannot increase memory by more than this factor * priming's increase (on average)"""
MEMORY_PRIMING_RUNS_N: int
"""How many times to run every test method to prime the heap"""
MEMORY_LEAK_CHECK_RUNS_N: int
"""How many times to run every test method to check for memory leaks"""

def _callTestMethod(self, method):
mem_original_b = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
for _ in range(self.MEMORY_PRIMING_RUNS_N): # Priming runs
method()
mem_primed_b = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
for _ in range(self.MEMORY_LEAK_CHECK_RUNS_N): # Memory leak check runs
method()
mem_tested_b = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
avg_memory_priming_increase_b = (mem_primed_b - mem_original_b) / self.MEMORY_PRIMING_RUNS_N
avg_memory_test_increase_b = (mem_tested_b - mem_primed_b) / self.MEMORY_LEAK_CHECK_RUNS_N
avg_memory_increase_factor = (
avg_memory_test_increase_b / avg_memory_priming_increase_b if avg_memory_priming_increase_b else 0
)
self.assertLessEqual( # type: ignore
avg_memory_test_increase_b,
self.MEMORY_INCREASE_PER_PARSE_LIMIT_B,
f"Possible memory leak - exceeded {self.MEMORY_INCREASE_PER_PARSE_LIMIT_B}-byte limit of incremental memory per parse",
)
self.assertLessEqual( # type: ignore
avg_memory_increase_factor,
self.MEMORY_INCREASE_INCREMENTAL_FACTOR_LIMIT,
f"Possible memory leak - exceeded {self.MEMORY_INCREASE_INCREMENTAL_FACTOR_LIMIT*100:.2f}% limit of incremental memory per parse",
)


class BaseTest(TestMixin, ErrorResponsesMixin, TestCase):
"""
Base class for performing Postgres-based backend unit tests on.
Expand Down
2 changes: 1 addition & 1 deletion requirements.in
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,4 @@ django-two-factor-auth==1.14.0
phonenumberslite==8.13.6
openai==0.27.8
nh3==0.2.14
hogql-parser==0.1.7
hogql-parser==0.1.8
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ gunicorn==20.1.0
# via -r requirements.in
h11==0.13.0
# via wsproto
hogql-parser==0.1.7
hogql-parser==0.1.8
# via -r requirements.in
idna==2.8
# via
Expand Down

0 comments on commit 711f2b4

Please sign in to comment.