Skip to content
15 changes: 11 additions & 4 deletions llvmlite/ir/_utils.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
from collections import defaultdict


class DuplicatedNameError(NameError):
Expand All @@ -8,7 +7,7 @@ class DuplicatedNameError(NameError):
class NameScope(object):
def __init__(self):
self._useset = set([''])
self._basenamemap = defaultdict(int)
self._basenamemap = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a defaultdict slower than a normal dict? It seems that making this change here requires you to then emulate a default dict below - is there a reason not to keep it as a defaultdict?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Follow-up - I tried reverting the changes in this file and it seemed to make no performance difference in test_debug_info_caching)


def is_used(self, name):
return name in self._useset
Expand All @@ -23,10 +22,18 @@ def register(self, name, deduplicate=False):

def deduplicate(self, name):
basename = name

try:
ident = self._basenamemap[basename]
except KeyError:
ident = 0
Comment on lines +26 to +29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the emulation of a default dict I mentioned in the comment above - could it just be

Suggested change
try:
ident = self._basenamemap[basename]
except KeyError:
ident = 0
ident = self._basenamemap[basename]

with _basenamemap remaining a defaultdict?


while self.is_used(name):
ident = self._basenamemap[basename] + 1
self._basenamemap[basename] = ident
ident += 1
name = "{0}.{1}".format(basename, ident)

self._basenamemap[basename] = ident

return name

def get_child(self):
Expand Down
4 changes: 2 additions & 2 deletions llvmlite/ir/builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ def wrapped(self, operand, flag, name=''):
raise TypeError(
"expected an integer type, got %s" %
operand.type)
if not(isinstance(flag.type, types.IntType) and
flag.type.width == 1):
if not (isinstance(flag.type, types.IntType) and
flag.type.width == 1):
raise TypeError("expected an i1 type, got %s" % flag.type)
fn = self.module.declare_intrinsic(
opname, [operand.type, flag.type])
Expand Down
39 changes: 23 additions & 16 deletions llvmlite/ir/module.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,17 @@ def add_metadata(self, operands):
if not isinstance(operands, (list, tuple)):
raise TypeError("expected a list or tuple of metadata values, "
"got %r" % (operands,))
operands = self._fix_metadata_operands(operands)
key = tuple(operands)
if key not in self._metadatacache:
n = len(self.metadata)
md = values.MDValue(self, operands, name=str(n))
self._metadatacache[key] = md
else:
md = self._metadatacache[key]

fixed_operands = self._fix_metadata_operands(operands)
key = hash(tuple(fixed_operands))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR description, I see

Only generates the key for module._metadatacache a single time instead of twice on cache miss

I'm not sure I can understand where it was generated twice before - am I missing that somewhere?

In any case, I do think the changes here are an improvement - reverting the specific changes to this file does slow down the performance test in test_debug_info_caching below.

try:
return self._metadatacache[key]
except KeyError:
pass

n = len(self.metadata)
md = values.MDValue(self, fixed_operands, name=str(n))
self._metadatacache[key] = md
return md

def add_debug_info(self, kind, operands, is_distinct=False):
Expand All @@ -72,14 +75,18 @@ def add_debug_info(self, kind, operands, is_distinct=False):
A DIValue instance is returned, it can then be associated to e.g.
an instruction.
"""
operands = tuple(sorted(self._fix_di_operands(operands.items())))
key = (kind, operands, is_distinct)
if key not in self._metadatacache:
n = len(self.metadata)
di = values.DIValue(self, is_distinct, kind, operands, name=str(n))
self._metadatacache[key] = di
else:
di = self._metadatacache[key]
fixed_operands = self._fix_di_operands(sorted(operands.items()))
key = hash((kind, tuple(fixed_operands), is_distinct))

try:
return self._metadatacache[key]
except KeyError:
pass

n = len(self.metadata)
di = values.DIValue(
self, is_distinct, kind, fixed_operands, name=str(n))
self._metadatacache[key] = di
return di

def add_named_metadata(self, name, element=None):
Expand Down
24 changes: 22 additions & 2 deletions llvmlite/ir/values.py
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,7 @@ def __init__(self, parent, values, name):
types.MetaDataType(),
name=name)
self.operands = tuple(values)
self.hash_cache = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like hash_cache is an implementation detail rather than an attribute we'd want to expose - should this be _hash_cache instead?

parent.metadata.append(self)

def descr(self, buf):
Expand Down Expand Up @@ -694,8 +695,17 @@ def __eq__(self, other):
def __ne__(self, other):
return not self.__eq__(other)

def __getstate__(self):
# Ensure that the hash is not cached between Python invocations
# due to pickling or other serialization. The hash seed changes
# which will cause these not to match.
self.hash_cache = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a bit odd to mutate state inside __getstate__ - do you see any downside to removing the hash cache from a copy of the dict?

return self.__dict__
Comment on lines +702 to +703
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.

Suggested change
self.hash_cache = None
return self.__dict__
state = self.__dict__.copy()
state['hash_cache'] = None
return state


def __hash__(self):
return hash(self.operands)
if self.hash_cache is None:
self.hash_cache = hash(self.operands)
return self.hash_cache


class DIToken:
Expand Down Expand Up @@ -725,6 +735,7 @@ def __init__(self, parent, is_distinct, kind, operands, name):
self.is_distinct = is_distinct
self.kind = kind
self.operands = tuple(operands)
self.hash_cache = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment on the name to above, i.e. should it be _hash_cache?

parent.metadata.append(self)

def descr(self, buf):
Expand Down Expand Up @@ -767,8 +778,17 @@ def __eq__(self, other):
def __ne__(self, other):
return not self.__eq__(other)

def __getstate__(self):
# Ensure that the hash is not cached between Python invocations
# due to pickling or other serialization. The hash seed changes
# which will cause these not to match.
self.hash_cache = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also should this invalidate the hash cache on a copy?

return self.__dict__

def __hash__(self):
return hash((self.is_distinct, self.kind, self.operands))
if self.hash_cache is None:
self.hash_cache = hash(self.operands)
return self.hash_cache


class GlobalValue(NamedValue, _ConstOpMixin, _HasMetadata):
Expand Down
89 changes: 89 additions & 0 deletions llvmlite/tests/test_ir.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import pickle
import re
import textwrap
import timeit
import unittest

from . import TestCase
Expand Down Expand Up @@ -526,6 +527,94 @@ def test_debug_info_unicode_string(self):
name = ''.join(map(lambda x: f"\\{x:02x}", "∆".encode()))
self.assert_ir_line(f'!0 = !DILocalVariable(name: "a{name}")', strmod)

def test_debug_info_caching(self):
mod = None
foo = None
builder = None
di_subprogram = None

def setup_test():
nonlocal mod, foo, builder, di_subprogram
mod = self.module()

di_file = mod.add_debug_info(
'DIFile',
{
'directory': 'my_directory',
'filename': 'my_path.foo',
})

di_compile_unit = mod.add_debug_info(
'DICompileUnit',
{
'emissionKind': ir.DIToken('FullDebug'),
'file': di_file,
'isOptimized': False,
'language': ir.DIToken('DW_LANG_C99'),
'producer': 'llvmlite-test',
})

di_subprogram = mod.add_debug_info(
'DISubprogram',
{
'file': di_file,
'line': 123,
'name': 'my function',
'scope': di_file,
'scopeLine': 123,
'unit': di_compile_unit,
})

foo = ir.Function(mod, ir.FunctionType(ir.VoidType(), []), 'foo')
builder = ir.IRBuilder(foo.append_basic_block(''))

def do_test():
for i in range(100_000):
di_location = mod.add_debug_info(
'DILocation',
{
'scope': di_subprogram,
'line': i,
'column': 15,
'other': [di_subprogram, di_subprogram],
})

builder.debug_metadata = di_location

builder.and_(
ir.Constant(ir.IntType(bits=32), i),
ir.Constant(ir.IntType(bits=32), i + 1))

total_time = timeit.timeit(
'do_test()',
'setup_test()',
number=10,
globals=locals())

print('test_debug_info_performance took', total_time, 'to finish')

self.assertEqual(100004, len(mod._metadatacache))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the expected result could do with a comment explaining its derivation - it took me a while to work out that it's 100000 locations, plus one file, one compile unit, one subprogram, and [di_subprogram, di_subprogram].


def test_debug_info_pickle(self):
mod = self.module()

di_file = mod.add_debug_info(
'DIFile',
{
'directory': 'my_directory',
'filename': 'my_path.foo',
})
self.assertEqual(hash(di_file), di_file.hash_cache)
found_di_file = pickle.loads(pickle.dumps(di_file))
self.assertIsNone(found_di_file.hash_cache)
self.assertEqual(di_file, found_di_file)

meta = mod.add_metadata([di_file])
self.assertEqual(hash(meta), meta.hash_cache)
found_meta = pickle.loads(pickle.dumps(meta))
self.assertIsNone(found_meta.hash_cache)
self.assertEqual(meta, found_meta)

def test_inline_assembly(self):
mod = self.module()
foo = ir.Function(mod, ir.FunctionType(ir.VoidType(), []), 'foo')
Expand Down