Дешевле обёртка вызова методов контекста - #1747
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe marshaller adds specialized conversions for selected primitive parameter and return types. Generated expressions call wrapped context methods directly and use converter selectors. Constructors also use the parameter converter selector. Tests cover conversions, defaults, overflow, and wrapped method calls. ChangesPrimitive conversion and context invocation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to The wrapper changes have no identified behavior regression and are ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The invocation path changes, but the reviewed code continues to call the same exposed methods, pass the same process context, and use existing conversion behavior for other values. No new security issue was established. Exception behavior for host methods that throw has not been compared end to end. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // ConvertParam<TypeOfArgN>(args[i], defaults[i])); | ||
| // ConvertParam<TypeOfArgN>(args[N-1], defaultN, process)); | ||
| // } | ||
| // Метод вызывается напрямую, без промежуточного делегата: это заметно дешевле на каждом вызове. |
There was a problem hiding this comment.
Кажется эту последнюю строчку комментария надо удалить. Она интересна в контексте текущего PR, но на будущее ее нет смысла хранить, т.к. она объясняет уже ушедшее из кода состояние (промежуточный делегат)
| public static T ConvertParam<T>(IValue value, T defaultValue, IBslProcess process) | ||
| { | ||
| // Частые случаи - без упаковки и перебора типов в ConvertValueType, результат тот же. | ||
| // Для значимых T проверки typeof JIT вычисляет при компиляции |
There was a problem hiding this comment.
Покорежило перевод. "значимых" имелся в виду value-type? Тогда надо другое слово, "примитивных" или "типов-значений" как-то так.
There was a problem hiding this comment.
Этого комментария больше нет: быстрые пути ушли из ConvertParam<T>, подробности в общем комментарии.
| public static IValue ConvertReturnValue<TRet>(TRet param) | ||
| { | ||
| // Частые значимые типы - без упаковки, проверки JIT вычисляет при компиляции | ||
| if (typeof(TRet) == typeof(bool)) |
There was a problem hiding this comment.
Не очень мне, конечно, нравится, что разъехалась в разные места логика конвертации примитивов, но чего не сделаешь ради производительности...
There was a problem hiding this comment.
В новой версии быстрые пути собраны в одном месте — в начале ContextValuesMarshaller, рядом с выбором. И всё, что не подошло по виду значения, они отдают в общий ConvertParam<T>, так что правила преобразования по-прежнему в одном месте.
285534b to
626ed2f
Compare
|
Перепроверил BenchmarkDotNet (бенч-проект пришлю отдельным PR) и нашёл у себя ошибку: проверки Переделал: преобразование выбирается один раз, при построении обёртки ( |
626ed2f to
b8d046b
Compare
| { | ||
| private static readonly Dictionary<Type, MethodInfo> _primitiveParameterConverters = new Dictionary<Type, MethodInfo> | ||
| { | ||
| [typeof(int)] = GetOwnMethod(nameof(ConvertInt32Param)), |
There was a problem hiding this comment.
обычный switch не будет тут выгоднее Dictionary?
There was a problem hiding this comment.
Выбор делается один раз — при построении обёртки метода, а не на вызове, так что скорость тут не важна. Словарь — ради точного совпадения типа: switch по Type.GetTypeCode отнёс бы к int и перечисления с базовым int, а цепочка if по typeof вышла бы длиннее. Если читается хуже — переделаю на if.
Скомпилированная лямбда вызывает C#-метод напрямую, а не через делегат из замыкания - делегат остался от ручных оберток до перехода на деревья выражений. Для параметров int/decimal/bool/string и возврата bool/int/decimal обертка при построении выбирает отдельные методы преобразования: значение нужного вида берется сразу, без упаковки и перебора типов, остальное идет прежним путем ConvertParam<T>. Так же собираются конструкторы в TypeFactory. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
b8d046b to
4450f74
Compare
2 participants
Обёртка, через которую машина вызывает методы контекста, стоила 25–33 нс на вызов сверх самого метода. Две причины:
CreateFunction<T1, T2, …>, которые были до перехода на деревья выражений (9e82cea); в дереве он не нужен — теперьExpression.Callпрямо на методе;ConvertParam<T>на каждый аргумент шёл через необобщённыйConvertValueType:Nullable.GetUnderlyingType, сравнениеSystemTypeпо строковому id, цепочкаtype == typeof(...)и упаковка числа вobject.ConvertReturnValue<T>упаковывалbool/intи разбирал их switch-ем.Теперь преобразование выбирается один раз, при построении обёртки (
ContextValuesMarshaller.GetParameterConverter/GetReturnValueConverter). Для параметровint/decimal/bool/stringэто отдельные методы: значение нужного вида берётся сразу, остальное (пропущенный аргумент, ссылка на переменную,Неопределено, строка вместо числа) идёт прежнимConvertParam<T>. Возвратbool/int/decimal— без упаковки. ПубличныеConvertParam<T>/ConvertReturnValue<T>не менялись. Так же собираются конструкторы вTypeFactory. ТестContextValuesMarshallerTestна граничные случаи проходит и на develop, и на ветке.Итого: сама обёртка на вызовах с числами дешевле на 30–40% и без аллокаций, на
IValue— на 7%. В скриптах — −4…−8% там, где в вызове числа (Массив.Количество,Массив.Установить,СтрНайти,Рефлектор.ВызватьМетод), и на 24 Б меньше аллокаций на вызов; остальное в шуме, пиковая память та же.Вместе с #1746 почти закрывает регрессию
Заблокировать()из #1745 (проверял локальным слиянием трёх веток): пара «заблокировать/разблокировать» за вычетом пустого цикла — 141 нс на develop, 211 нс на #1745, 158 нс со всеми тремя. Остаток — реестр блокировок и внедрение процесса.Бенчи. BenchmarkDotNet 0.14 (проект — #1748), .NET 8, i7-13700KF, Windows 11. По 3 запуска на версию: JIT от процесса к процессу компилирует горячий путь обёртки то в быстрый, то в медленный вариант — у обеих версий, — и в одном запуске разница бывает случайной.
BenchmarkDotNet: develop → ветка, время и аллокации на операцию
Обёртка, вызов из C#:
Массив.Установить(0, x)Массив.Количество()Массив.Получить(0)Соответствие.Получить(x)Из 1Script, стековая машина, на итерацию цикла:
Заблокировать()+Разблокировать()Массив.Установить(0, Сч)Р = Массив.Количество()Массив.Добавить(Сч)Структура.Вставить("Ключ", Сч)Р = Соответствие.Получить(1)Р = СтрНайти("строка", "к")Р = Сложить(Сч, 1)— функция скриптаР = СПоУмолчанию(Сч)Р = ЭтотОбъект.Сложить(Сч, 1)Рефлектор.ВызватьМетод(М, "Установить", …)Функции и методы скрипта обёртку не затрагивают — это контроль; разница до ~3% между сборками — шум раскладки кода.
Весь процесс: время, CPU, аллокации, сборки, пиковая память (develop → ветка)
Каждая нагрузка — отдельный процесс
oscript, 4 прогона на прогрев + 4 замера, 5 раундов, медианы; CPU и память — по всему процессу черезDOTNET_STARTUP_HOOKS.¹
СтрДлинакомпилируется во встроенную инструкцию, а не в вызов метода — контрольная нагрузка.² Процесс попадает то в режим ~338 мс, то в ~365 мс — у обеих сборок; здесь develop попал в быстрый режим в 4 раундах из 5, ветка — в 2. BenchmarkDotNet на тех же сборках: 0,97.
🤖 Generated with Claude Code
Summary by CodeRabbit