- 
                Notifications
    
You must be signed in to change notification settings  - Fork 467
 
          feat(profiling): [unwrapt] remove wrapt dependency from Lock Profiler
          #15003
        
          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
base: main
Are you sure you want to change the base?
Conversation
| 
           
  | 
    
wrapt
      wraptwrapt
      
          Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 237 ± 2 ms. The average import time from base is: 240 ± 2 ms. The import time difference between this PR and base is: -2.73 ± 0.09 ms. Import time breakdownThe following import paths have shrunk: 
             | 
    
          Performance SLOsComparing candidate vlad/lock-profiler-un-wrapted (6c0c3d6) with baseline main (0a50af6) 📈 Performance Regressions (1 suite)📈 iastaspectsospath - 24/24✅ ospathbasename_aspectTime: ✅ 4.310µs (SLO: <10.000µs 📉 -56.9%) vs baseline: +1.1% Memory: ✅ 37.473MB (SLO: <39.000MB -3.9%) vs baseline: +5.1% ✅ ospathbasename_noaspectTime: ✅ 1.088µs (SLO: <10.000µs 📉 -89.1%) vs baseline: +0.4% Memory: ✅ 37.415MB (SLO: <39.000MB -4.1%) vs baseline: +4.9% ✅ ospathjoin_aspectTime: ✅ 6.973µs (SLO: <10.000µs 📉 -30.3%) vs baseline: 📈 +12.8% Memory: ✅ 37.434MB (SLO: <39.000MB -4.0%) vs baseline: +4.9% ✅ ospathjoin_noaspectTime: ✅ 2.317µs (SLO: <10.000µs 📉 -76.8%) vs baseline: +0.6% Memory: ✅ 37.454MB (SLO: <39.000MB -4.0%) vs baseline: +5.0% ✅ ospathnormcase_aspectTime: ✅ 3.527µs (SLO: <10.000µs 📉 -64.7%) vs baseline: -0.5% Memory: ✅ 37.434MB (SLO: <39.000MB -4.0%) vs baseline: +4.9% ✅ ospathnormcase_noaspectTime: ✅ 0.565µs (SLO: <10.000µs 📉 -94.3%) vs baseline: -1.0% Memory: ✅ 37.375MB (SLO: <39.000MB -4.2%) vs baseline: +4.6% ✅ ospathsplit_aspectTime: ✅ 4.869µs (SLO: <10.000µs 📉 -51.3%) vs baseline: +1.3% Memory: ✅ 37.473MB (SLO: <39.000MB -3.9%) vs baseline: +5.1% ✅ ospathsplit_noaspectTime: ✅ 1.591µs (SLO: <10.000µs 📉 -84.1%) vs baseline: +0.4% Memory: ✅ 37.434MB (SLO: <39.000MB -4.0%) vs baseline: +4.8% ✅ ospathsplitdrive_aspectTime: ✅ 3.738µs (SLO: <10.000µs 📉 -62.6%) vs baseline: +0.6% Memory: ✅ 37.454MB (SLO: <39.000MB -4.0%) vs baseline: +5.1% ✅ ospathsplitdrive_noaspectTime: ✅ 0.696µs (SLO: <10.000µs 📉 -93.0%) vs baseline: -0.2% Memory: ✅ 37.395MB (SLO: <39.000MB -4.1%) vs baseline: +4.7% ✅ ospathsplitext_aspectTime: ✅ 4.601µs (SLO: <10.000µs 📉 -54.0%) vs baseline: +0.8% Memory: ✅ 37.473MB (SLO: <39.000MB -3.9%) vs baseline: +5.0% ✅ ospathsplitext_noaspectTime: ✅ 1.387µs (SLO: <10.000µs 📉 -86.1%) vs baseline: ~same Memory: ✅ 37.434MB (SLO: <39.000MB -4.0%) vs baseline: +5.0% 🟡 Near SLO Breach (4 suites)🟡 djangosimple - 30/30✅ appsecTime: ✅ 20.495ms (SLO: <22.300ms -8.1%) vs baseline: +0.1% Memory: ✅ 66.080MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +4.9% ✅ exception-replay-enabledTime: ✅ 1.339ms (SLO: <1.450ms -7.7%) vs baseline: -0.4% Memory: ✅ 64.079MB (SLO: <67.000MB -4.4%) vs baseline: +4.7% ✅ iastTime: ✅ 20.420ms (SLO: <22.250ms -8.2%) vs baseline: ~same Memory: ✅ 66.119MB (SLO: <67.000MB 🟡 -1.3%) vs baseline: +5.0% ✅ profilerTime: ✅ 15.416ms (SLO: <16.550ms -6.9%) vs baseline: -1.0% Memory: ✅ 53.856MB (SLO: <54.500MB 🟡 -1.2%) vs baseline: +4.9% ✅ resource-renamingTime: ✅ 20.532ms (SLO: <21.750ms -5.6%) vs baseline: -0.2% Memory: ✅ 66.119MB (SLO: <67.000MB 🟡 -1.3%) vs baseline: +5.1% ✅ span-code-originTime: ✅ 25.369ms (SLO: <28.200ms 📉 -10.0%) vs baseline: -0.3% Memory: ✅ 67.802MB (SLO: <69.500MB -2.4%) vs baseline: +5.9% ✅ tracerTime: ✅ 20.426ms (SLO: <21.750ms -6.1%) vs baseline: ~same Memory: ✅ 66.041MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +4.9% ✅ tracer-and-profilerTime: ✅ 22.510ms (SLO: <23.500ms -4.2%) vs baseline: -0.5% Memory: ✅ 67.476MB (SLO: <68.000MB 🟡 -0.8%) vs baseline: +4.9% ✅ tracer-dont-create-db-spansTime: ✅ 19.262ms (SLO: <21.500ms 📉 -10.4%) vs baseline: -0.5% Memory: ✅ 66.021MB (SLO: <67.000MB 🟡 -1.5%) vs baseline: +4.8% ✅ tracer-minimalTime: ✅ 16.581ms (SLO: <17.500ms -5.3%) vs baseline: ~same Memory: ✅ 65.706MB (SLO: <67.000MB 🟡 -1.9%) vs baseline: +4.2% ✅ tracer-nativeTime: ✅ 20.467ms (SLO: <21.750ms -5.9%) vs baseline: +0.3% Memory: ✅ 71.488MB (SLO: <72.500MB 🟡 -1.4%) vs baseline: +4.9% ✅ tracer-no-cachesTime: ✅ 18.506ms (SLO: <19.650ms -5.8%) vs baseline: +0.4% Memory: ✅ 66.014MB (SLO: <67.000MB 🟡 -1.5%) vs baseline: +4.8% ✅ tracer-no-databasesTime: ✅ 18.793ms (SLO: <20.100ms -6.5%) vs baseline: ~same Memory: ✅ 65.706MB (SLO: <67.000MB 🟡 -1.9%) vs baseline: +4.7% ✅ tracer-no-middlewareTime: ✅ 20.158ms (SLO: <21.500ms -6.2%) vs baseline: -0.2% Memory: ✅ 66.080MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +5.0% ✅ tracer-no-templatesTime: ✅ 20.271ms (SLO: <22.000ms -7.9%) vs baseline: -0.4% Memory: ✅ 66.060MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +5.0% 🟡 errortrackingdjangosimple - 6/6✅ errortracking-enabled-allTime: ✅ 18.004ms (SLO: <19.850ms -9.3%) vs baseline: -0.3% Memory: ✅ 66.060MB (SLO: <66.500MB 🟡 -0.7%) vs baseline: +5.0% ✅ errortracking-enabled-userTime: ✅ 18.018ms (SLO: <19.400ms -7.1%) vs baseline: -0.1% Memory: ✅ 65.994MB (SLO: <66.500MB 🟡 -0.8%) vs baseline: +5.0% ✅ tracer-enabledTime: ✅ 18.058ms (SLO: <19.450ms -7.2%) vs baseline: +0.4% Memory: ✅ 65.667MB (SLO: <66.500MB 🟡 -1.3%) vs baseline: +4.8% 🟡 flasksimple - 18/18✅ appsec-getTime: ✅ 4.588ms (SLO: <4.750ms -3.4%) vs baseline: +0.2% Memory: ✅ 62.229MB (SLO: <65.000MB -4.3%) vs baseline: +4.8% ✅ appsec-postTime: ✅ 6.611ms (SLO: <6.750ms -2.1%) vs baseline: -0.2% Memory: ✅ 62.288MB (SLO: <65.000MB -4.2%) vs baseline: +5.0% ✅ appsec-telemetryTime: ✅ 4.586ms (SLO: <4.750ms -3.5%) vs baseline: -0.3% Memory: ✅ 62.225MB (SLO: <65.000MB -4.3%) vs baseline: +4.7% ✅ debuggerTime: ✅ 1.853ms (SLO: <2.000ms -7.4%) vs baseline: ~same Memory: ✅ 45.205MB (SLO: <47.000MB -3.8%) vs baseline: +5.1% ✅ iast-getTime: ✅ 1.855ms (SLO: <2.000ms -7.3%) vs baseline: -0.3% Memory: ✅ 41.964MB (SLO: <49.000MB 📉 -14.4%) vs baseline: +4.6% ✅ profilerTime: ✅ 1.915ms (SLO: <2.100ms -8.8%) vs baseline: +0.2% Memory: ✅ 46.453MB (SLO: <47.000MB 🟡 -1.2%) vs baseline: +5.0% ✅ resource-renamingTime: ✅ 3.370ms (SLO: <3.650ms -7.7%) vs baseline: +0.4% Memory: ✅ 52.510MB (SLO: <53.500MB 🟡 -1.9%) vs baseline: +4.8% ✅ tracerTime: ✅ 3.351ms (SLO: <3.650ms -8.2%) vs baseline: ~same Memory: ✅ 52.487MB (SLO: <53.500MB 🟡 -1.9%) vs baseline: +4.9% ✅ tracer-nativeTime: ✅ 3.344ms (SLO: <3.650ms -8.4%) vs baseline: -0.4% Memory: ✅ 58.117MB (SLO: <60.000MB -3.1%) vs baseline: +4.9% 🟡 flasksqli - 6/6✅ appsec-enabledTime: ✅ 3.971ms (SLO: <4.200ms -5.4%) vs baseline: +0.4% Memory: ✅ 62.325MB (SLO: <66.000MB -5.6%) vs baseline: +4.8% ✅ iast-enabledTime: ✅ 2.448ms (SLO: <2.800ms 📉 -12.6%) vs baseline: +0.8% Memory: ✅ 59.159MB (SLO: <60.000MB 🟡 -1.4%) vs baseline: +4.9% ✅ tracer-enabledTime: ✅ 2.059ms (SLO: <2.250ms -8.5%) vs baseline: -0.2% Memory: ✅ 52.534MB (SLO: <54.500MB -3.6%) vs baseline: +4.8% 
 | 
    
| 
           test comment  | 
    
d5f6245    to
    897d642      
    Compare
  
    | 
           test  | 
    
cba7ce2    to
    ba19aff      
    Compare
  
    wraptwrapt dependency from lock profiler
      wrapt dependency from lock profilerwrapt dependency from Lock Profiler
      d34e7f1    to
    90d1d64      
    Compare
  
    wrapt dependency from Lock Profilerwrapt dependency from Lock Profiler
      9914135    to
    2778109      
    Compare
  
    wrapt dependency from Lock Profilerwrapt dependency from Lock Profiler
      wrapt dependency from Lock Profilerwrapt dependency from Lock Profiler
      c240aa2    to
    ab97e9b      
    Compare
  
    ab97e9b    to
    c16cdc1      
    Compare
  
    | # The underlying threading.Lock class is implemented using C code, and | ||
| # it doesn't have the __dict__ attribute. So we can't do | ||
| # self.__dict__.pop("_self_acquired_at", None) to remove the attribute. | ||
| # Instead, we need to use the following workaround to retrieve and | ||
| # remove the attribute. | ||
| start: Optional[int] = getattr(self, "_self_acquired_at", None) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you're removed the comment – is it not applicable anymore? (I guess so!)
If so, would it make sense to use __dict__ – does it make any different performance-wise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR - it is not appropriate anymore due use of special __slots__ attribute to store the wrapped lock's attributes (not a dict!)
details:
This comment had the right explanation, it was just incomplete. When wrapt wraps a C object, it has no dict attribte. When it wraps a Python obj - it does have dict!
So the comment was lacking this crucial detail, which makes it confusing, IMO. It should've said the following at the end:
"we need to use the following workaround to retrieve~ and remove ~the attribute."
-->
"we need to use the following workaround to retrieve the attribute due to race conditions" -- see the comment below that's explaining what happens.
33b44f4    to
    6f8c5c5      
    Compare
  
    86241a3    to
    1838825      
    Compare
  
    33809c6    to
    6c0c3d6      
    Compare
  
    
https://datadoghq.atlassian.net/browse/PROF-12854
Description
This PR removes
wraptdependency from lock profiler - replaced with direct delegation. It implements the following proposal in the Lock Profiler RFC: Remove wrapt dependencyWhy
wrapt's compiled C extension vs pure Python)Changes
wrapt.ObjectProxywith internal wrapper making use of__slots__(special class attribute that allows you to explicitly declare the data members (attributes) of a class, without using
dictimplicitly)_LockAllocatorWrapperas protocolPerformance
Ran benchmarks with 10K locks under multi-threaded contention:
See for details.
Lost Features
wraptis very powerful, but is an overkill for the Lock profiler in particular. That said, here are the features we would lose, which will be easy to implement ourselves:isinstance()checksdir(),vars(), etc.)Testing
test_patchfor new behavior + removed unnecessary test)vlad/benchmark-lock-profilerRisks
low: existing functionality intact, as shown by unit / e2e tests and local testing
low: could break users with esoteric workflows