Include class in region name

[Flamefire]

The current region name format is <module>:<function> which is sufficient for many use cases. It falls short when Python classes are involved especially when a Python class method has the same name as a module scoped function.

Example inspired by numpy:

class Foo:
  def bar(self):
    # do something

def bar(foo):
  foo.bar()

The resulting trace will look like the function bar called itself as only 1 region is created named module:bar which is found when the ENTER of foo.bar() is called.

This can be mitigated if e.g. co_firstlineno (which sounds like the line where the function is defined) is used as an additional key part in the map.

Even better would be the inclusion of the class name in the region name like: <module>[.<class>]:<function> which also helps to read and understand the trace. For example I was surprised to see a region named "_pydev_bundle.pydev_monkey:call" as pydev_monkey is a file, so how could a file be called?

[AndreasGocht]

Hey,

Yeah, that's a problem. I never have encountered this before.

As I understand something like the following is possible in python:

import foo as foo_module

class foo:
  def bar(self):
    # do something

def bar(foo):
  foo.bar()

So I would propose to do something like <module>[:<class>]:<function>. So the difference between a class and a module is obvious.
However, I have to figure out, how to get the information about a class.

Best,

Andreas

[Flamefire]

I found that when searching for _pydev_bundle.pydev_monkey:__call__ which confused me a lot. But it applies to all code using classes: It may not be wrong but hard to understand when reading the trace/profile.

Good point using a colon instead of point. I wanted to use a point as you usually do e.g.: scorep.instrumenters.ScorepTrace hence treat the class like a module. But then again the same applies to functions and the existing convention here is to use a colon so it makes sense to apply this to classes too.

I'm currently searching for example code I found earlier on how to get a class from a frame. It is certainly possible just need to find it again.

[AndreasGocht]

Turns out, that this is not so easy.

Getting the class name is indeed possible, e.g. by doing something like:

frame.f_locals["self"].__class__.__name__

However, first, it is not necessary that the variable is called self:

class TestClass:
    def doo(arg):
        print("doo")
t = TestClass()
t.doo()

is valid python code.

Moreover, it's not an issue with classes but with namespaces in python. If you have something like:

def foo():
    print("foo")
    def bar(arg):
        print(arg)
    doo("test")

def bar(arg):
     print(arg)

you'll end up in the same situation. I haven't found a way to get the qualified namespace.

For completeness: Since Python 3.3 there is a __qualname__, which can be called on a "class, function or method". However, we get a frame, which allows us to get a code object, but:

The difference between a code object and a function object is that the function object contains an explicit reference to the function’s globals (the module in which it was defined), while a code object contains no context; also the default argument values are stored in the function object, not in the code object (because they represent values calculated at run-time).

https://docs.python.org/3.8/reference/datamodel.html#the-standard-type-hierarchy

So we cannot access the qualified name as easy.

[Flamefire]

I see. For reference this is the output of some simple code using cProfile.run (Python builtin):

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.004    0.004 <string>:1(<module>)
        1    0.000    0.000    0.000    0.000 posixpath.py:144(basename)
        2    0.000    0.000    0.000    0.000 posixpath.py:41(_get_sep)
        1    0.000    0.000    0.000    0.000 posixpath.py:64(isabs)
        3    0.000    0.000    0.000    0.000 pydevd.py:752(_internal_get_file_type)
      5/4    0.000    0.000    0.000    0.000 pydevd.py:798(get_file_type)
        4    0.000    0.000    0.000    0.000 pydevd.py:873(get_cache_file_type)
        3    0.000    0.000    0.000    0.000 pydevd_api.py:818(custom_dont_trace_external_files)
        1    0.000    0.000    0.000    0.000 pydevd_file_utils.py:348(_NormPaths)
        2    0.000    0.000    0.000    0.000 pydevd_file_utils.py:391(_NormPath)
        2    0.000    0.000    0.000    0.000 pydevd_file_utils.py:753(get_abs_path_real_path_and_base_from_file)
        1    0.004    0.004    0.004    0.004 test_tracer.py:35(bar)
        1    0.000    0.000    0.000    0.000 test_tracer.py:43(foo)
        1    0.000    0.000    0.004    0.004 test_tracer.py:47(bar)
        1    0.000    0.000    0.004    0.004 {built-in method builtins.exec}
        2    0.000    0.000    0.000    0.000 {built-in method builtins.isinstance}
        1    0.000    0.000    0.000    0.000 {built-in method builtins.print}
        2    0.000    0.000    0.000    0.000 {built-in method posix.fspath}
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}
        5    0.000    0.000    0.000    0.000 {method 'endswith' of 'str' objects}
        3    0.000    0.000    0.000    0.000 {method 'get' of 'dict' objects}
        1    0.000    0.000    0.000    0.000 {method 'rfind' of 'str' objects}
       15    0.000    0.000    0.000    0.000 {method 'startswith' of 'str' objects}

So even they don't get the full name. However they do get it for C-calls where a funcobject is passed (we don't currently trace C-calls. but likely should or are they covered by Score-P instead?)

So I'd say we can keep the region names as we currently have them and ignore the visual name clash on functions with the same name but in different scopes.

However we must handle the name clash at the lower level: As the region name only is used as a key into the map the different functions are the same region. Adding the line number to the key might help already.

I'd also like to propose using the codeobject pointer as a key. That is what the builtin profiler does and it might improve performance a lot as it would skip building any strings altogether most of the time and using a string in a hash map is also quite slow. Might need some discussion prior though e.g. when mixing with OA-regions and the current/old Python instrumenters.

[AndreasGocht]

However we must handle the name clash at the lower level: As the region name only is used as a key into the map the different functions are the same region. Adding the line number to the key might help already.

I am aware of that 😉 .

I'd also like to propose using the codeobject pointer as a key. That is what the builtin profiler does and it might improve performance a lot as it would skip building any strings altogether most of the time and using a string in a hash map is also quite slow. Might need some discussion prior though e.g. when mixing with OA-regions and the current/old Python instrumenters.

Interesting Idea. But then we would need to handle user regions differently. I'll come up with a solution for this.

Best,

Andreas

[AndreasGocht]

Interesting Idea. But then we would need to handle user regions differently. I'll come up with a solution for this.

this could be done using id:

Return the “identity” of an object. This is an integer which is guaranteed to be unique and constant for this object during its lifetime. Two objects with non-overlapping lifetimes may have the same id() value.

CPython implementation detail: This is the address of the object in memory.

However, this brakes, if someone uses importlib.reload, see 17fec66 and 35daf29
I have to think about this.

[Flamefire]

However, this brakes, if someone uses importlib.reload, see 17fec66 and 35daf29

Not sure what the link to the first commit should tell me. But from what I understood: importlib.reload changes the id/pointer. I think one could argue that a) a reload is rare and b) after a reload the function is no longer the same hence creating a new region is fine.

Another strategy: Use the pointer to look up a region in the cache. If that fails, lookup using the region name as before and if that fails recreate. Basically a 2-stage cache. I think the cost for the 2nd lookup on first call is already paid off by a much cheaper lookup on every following call.

[AndreasGocht]

a reload the function is no longer the same hence creating a new region is fine.

Jip. But you might get two times the same function, with totally different behaviour. It's more that I like to detect these situations and inform the user, that this will break. Using your approach, I would like to come up with a slightly different one:

• check if the id is in the map
• if not create a new region, with a new region-name
• check if that region name is already in use with a different id
  • if so, print a warning
• business as usual

Moreover, I'd like to add the possibility to add the id additional to the line number to the region name, probably by using an environment variable.

[Flamefire]

It's more that I like to detect these situations and inform the user, that this will break.

"Break" in what sense? If you use the id (only) you get 1 physical region per actual region. If you reload there will be 2 regions although with the same name but e.g. vampir will not group them together. So all is fine.
The only point worth discussing is how to merge with functions instrumented by @scorep.user.region(), so if they should be the same as if they were instrumented by the instrumenter which is why I proposed the 2nd level cache. If we don't want to merge them than separate 1 lvl caches are fine.

check if that region name is already in use with a different id

This requires that the linenumber is included or you'll risk false warnings. It also needs adaption for user-instrumented regions or those will always print a warning.

Moreover, I'd like to add the possibility to add the id additional to the line number to the region name, probably by using an environment variable.

You mean so the region name would be module:function#lineNo#id? IMO that would look very messy in analyzers. I'd just add the lineNo as a key in the map (2nd level cache as above) to disambiguate regions with the same name for the purpose of recording them. The line number is already contained in the region description.

[AndreasGocht]

"Break" in what sense? If you use the id (only) you get 1 physical region per actual region. If you reload there will be 2 regions although with the same name but e.g. vampir will not group them together. So all is fine.

But you do not have a way to identify the different regions. But having two times the same region in the final trace will be a good indicator for reloading. I think this is sufficient.

The only point worth discussing is how to merge with functions instrumented by @scorep.user.region(), so if they should be the same as if they were instrumented by the instrumenter which is why I proposed the 2nd level cache. If we don't want to merge them than separate 1 lvl caches are fine.

I would simply use a sperate map. A region is either a user region or an instrumented region. Even the way we get the required information is different. So I think that's fine. I'll come up with an implementation.

You mean so the region name would be module:function#lineNo#id? IMO that would look very messy in analyzers. I'd just add the lineNo as a key in the map (2nd level cache as above) to disambiguate regions with the same name for the purpose of recording them. The line number is already contained in the region description.

Jip. That's why you would have to enable it explicitly. But It might be better so simply wait for a usecase here.

[AndreasGocht]

fixed with #105