Use the codeobject pointer to idetify regions

[AndreasGocht]

As discussed in #88, we now used the pointer to the python code object to identify a region. This allows to differentiate regions in different python namespaces.

Moreover, this should decrease the runtime, as the region_name is only created as needed. This patch might also have effects to #67.

Special care is needed for user regions, as here the line number is important for identifying a region. I choose only to add the line number there. I am not sure if it is worth to add the line number also to a region name.

Moreover, we simply cannot support importlib.relaod(). If all information of two regions, including file name and line number, are the same, Score-P merges them to one region.

[Flamefire]

I'm having trouble understanding the meaning of the 3 maps, maybe a couple comments describing the intent would help here.

What I get:

• "user region": Manually annotated region, i.e. not created by instrumenter
• Any user region is identified by the string <module-name>::<function-name>, let's call it region_id
• For any such region there is an entry in user_regions with region_id keys
• Foreach "instrumented region" (created by instrumenter) there is an entry in regions identified by it's code pointer
• If an instrumented region is entered the first time a user region of a matching region_id is searched for and used if found
• a translation of (region_id, lineNo)-tuples to regions is added for instrumented regions
• this translation is used before creating a new user region

As a result there can only be 1 user region with a given region_id.

I feel this can cause an issue and order-dependent results. Example (same region_id with different line numbers):

• auto region 1 -> New Region ID 1
• user region 1 -> Reuse Region ID 1
• auto region 2 -> Reuse Region ID 1 from user region

OR:

• auto region 1 -> new region id 1
• auto region 2 -> new region id 2
• user region 1 -> reuse region id 1

I feel like we can do a bit better. What (likely) cannot be done is that user region 1 and user region 2 can be differentiated, that would need a change to the region_end event to include the line number but I think we can at least always have the auto/instrumented region refer to correct/unique regions.
Idea: Write to region_translations in the user-region-enter and use that instead of the user_regions map in the instrumented-region-enter. I think this solves this problem as it merges instrumented and user regions only when the line numbers match no matter the order

2 final change requests:

1. have actual region handles in the region_translations map. This saves an extra map lookup
2. how about making the region_translations key a pair of (string, int)? IIRC std::pair is already hashable and this saves an int-to-string conversion on lookup and makes the key format clear.

[AndreasGocht]

The main point here ist to deal with decorators. Annotating a function:

@scorep.user.region()
def foo():
    pass

will instrument the region. If the instrumenter is enabled, the user instrumentation will do nothing. If the instrumenter is disabled, the user instrumentation will take care. Calling

foo()
with scorep.instrumenter.disable():
    foo()
    with scorep.instrumenter.enable():
         foo()

is supposed to refer to the same region, no matter if the instrumenter was initially enabled or not.

At the same time, instrumenting a region using

scorep.user.region_begin("bla")
# some code
scorep.user.region_end("bla")

is supposed to work.

I feel like we can do a bit better. What (likely) cannot be done is that user region 1 and user region 2 can be differentiated, that would need a change to the region_end event to include the line number but I think we can at least always have the auto/instrumented region refer to correct/unique regions.

We can do that for the decorator and the with statement. We cannot do that for the classical scorep.user.region_begin() and scorep.user.region_end() calls. But it might simply be the time to deprecate or remove them. Otherwise, we could simply force the user to provide a "line number". (Both would be a great reason for having a 4.0 release 😆 )

Idea: Write to region_translations in the user-region-enter and use that instead of the user_regions map in the instrumented-region-enter. I think this solves this problem as it merges instrumented and user regions only when the line numbers match no matter the order

I think if we address the above this should also be fine.

how about making the region_translations key a pair of (string, int)? IIRC std::pair is already hashable and this saves an int-to-string conversion on lookup and makes the key format clear.

Seems to be a good idea 😉.

[Flamefire]

I think if we address the above this should also be fine.

Ok so the only problem is that in in your example scorep.user.region_begin("bla") doesn't contain a line number, is this correct?

In this case I assume the line number passed into the C++ code is 0, isn't it? So my proposed solution would work: No "instrumented region" would be at line number 0 so it would never be gotten from the region_translations map. This is the correct behavior

The only issue I see is if the user uses something like:

scorep.user.region_begin("bla")
# some code
scorep.user.region_end("bla")

@scorep.user.region()
def bla():
    pass

What happens is: First begin creates a user-region "bla" and a mapping (bla, 0), second reuses the user-region but does not create a mapping. This will be a problem when the instrumenter gets called for bla as it will attribute calls to that function to a new region while when the instrumenter is disabled it will attribute calls to the custom region created before.

I'd call this suboptimal but as already written: User regions with the same region_id cannot be distinguished anyway so users should simply not do this.

[AndreasGocht]

I'd call this suboptimal but as already written: User regions with the same region_id cannot be distinguished anyway so users should simply not do this.

They can, as long as they use decorators and context manages, as they do have a line number. I'll come up with another code change, once we agreed on the other Issues.

Best,

Andreas

[Flamefire]

They can, as long as they use decorators and context manages, as they do have a line number. I'll come up with another code change

Not in the current implementation or am I missing anything? The decorator calls region_begin(self.module_name, self.region_name, full_file_name, line_number) so no function object (pointer) and the context manager and the manual scorep.user.region_begin("bla") will resolve to the same region, the line number is passed but not used for identification. For the latter this is expected and correct behavior anyway: Fold all user regions with the same name.

The decorator can be changed to include the wrapped functions code object which solves this case:

scorep.user.region_begin("bla")
# some code
scorep.user.region_end("bla")

@scorep.user.region()
def bla():
    pass

The first will create a user region, the second will include a function code pointer and resolve to a different region.

Given that: Why do we need the region_translations map? We actually only have 2 cases: Either a manually or automatically decorated function which includes the code pointer or a named user region identified by the name only and without a code pointer (created by a context manager or manual calls) I don't see any use case where those should be folded

This also gets rid of the cross-checks (checking for a user-region in the code-pointer-using function and vice-versa) and even better: We will create new regions for decorated or automatically instrumented functions on reload as the pointer changed and there is no name-matching fallback.

[AndreasGocht]

Given that: Why do we need the region_translations map? We actually only have 2 cases: Either a manually or automatically decorated function which includes the code pointer or a named user region identified by the name only and without a code pointer (created by a context manager or manual calls) I don't see any use case where those should be folded

done

[AndreasGocht]

I think I addressed all open Issues. If there are no objections, I'll merge this next week.

Best,

Andreas

[Flamefire]

Almost ready, just a few trivial changes