Skip to content
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

Feature/in memory db references to variables #1871

Merged

Conversation

qtLex
Copy link
Contributor

@qtLex qtLex commented Oct 12, 2021

Описание

Неиспользуемые переменные на ссылках

Связанные задачи

Closes #15

Чеклист

Общие

  • Ветка PR обновлена из develop
  • Отладочные, закомментированные и прочие, не имеющие смысла участки кода удалены
  • Изменения покрыты тестами
  • Обязательные действия перед коммитом выполнены (запускал команду gradlew precommit)

Для диагностик

  • Описание диагностики заполнено для обоих языков (присутствуют файлы для обоих языков, для русского заполнено все подробно, перевод на английский можно опустить)

Дополнительно

);
}

private boolean isErrorNode(BSLParser.SubContext ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

Trees.nodeContainsError не подошла?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Я еще сомневаюсь в этом решении. Вызов isErrorNode нужен для определения методов без символов из-за ошибок разбора. Я выбирал между Trees.nodeContainsError и реализацией в MethodSymbolComputer.visitFunction. Остановился на втором варианте так как хотелось получить одинаковое поведение в любой ситуации.

Возможно Trees.nodeContainsError не был реализован когда создавался MethodSymbolComputer, тогда можно заменить в обоих местах на Trees.nodeContainsError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Проверил. Trees.nodeContainsError и isErrorNode результат выдают разный. Trees.nodeContainsError не подошла.

Copy link
Member

Choose a reason for hiding this comment

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

а сложилось понимание, в чем отличие? возможно в methodSymbolComputer старое поведение, которое писалось еще на старом ANTLR, который был менее толерантен к ошибкам разбора внутри метода.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Я не посмотрел ((. "Вонзил" в Trees.nodeContainsError вместо isErrorNode и стало валится. Сейчас на свежую голову попробовал - работает!? Может какие изменения повлияли, а может что неправильно делал. Тесты вроде не менял.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Нашел свою старую заметку из первого подхода. Перепроверю:

Метод SymbolTree.getMethodSymbol(BSLParserRuleContext ctx) не может найти метод в дереве символов.

Код содержится в тесте на проверку ошибки разбора в фикстуре src/test/resources/context/computer/MethodSymbolComputerTestParseError.bsl.

Процедура Выполнить()

КонецПроцедуры

При этом проверка Trees.nodeContainsErrors(ctx) ошибку разбора не видит. При поиске получается ренж с именем в котором позиция конца раньше начала.

nixel2007 and others added 20 commits October 27, 2021 14:04
Spring-boot смотрит на интерфейсы
индексы, derived queries, EAGER чтение символа
Добавлена возможность отличить ссылку
на переопределение переменной от использования
@qtLex qtLex marked this pull request as ready for review November 4, 2021 20:39
@qtLex
Copy link
Contributor Author

qtLex commented Nov 4, 2021

Основное закончил. Можно кидаться тапками.

Еще ковыряюсь с производительностью, но кажется ищу где-то не там. Понемногу получается выиграть, но на больших конфигурациях. На БСП не заметно почти. Если будет, что то толковое получаться, думаю, будет лучше отдельный pr организовать.

@@ -57,6 +64,16 @@
@Getter(lazy = true)
List<MethodSymbol> methods = createMethods();

/**
* Список переменных документа
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Список переменных документа
* Плоский список всех переменных документа

Copy link
Member

Choose a reason for hiding this comment

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

Мне кажется, или у нас нет готового метода для переменных верхнего уровня? может быть этот метод переименовать в getVariablesFlat, а в пару к нему добавить getModuleLevelVariables, по аналогии с regions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Не уверен, что это хорошая идея.

Переменные уровня модуля по стандарту, если его конечно соблюдать, будут завернуты в область, а может и не одну. Чтоб его получить придется построить весь список переменных и его фильтрануть по виду переменной. Выигрыша в производительности не получиться как с regions, когда смотрится только первый уровень.

Плюс не вижу диагностик где это может понадобиться.

@qtLex
Copy link
Contributor Author

qtLex commented Nov 5, 2021

Nikita Gryzlov, [05.11.2021 11:58]
@qtLex кстати, есть вариант, как "обмануть" бэнч и в целом оптимизировать инициализацию контекста. сейчас референс индекс и символьное дерево при анализе по сути инициализируются по два раза на каждый файл - первоначально при populateContext они заполняются для всех файлов, а потом при анализе для каждого файла вызывается rebuild с version = 0, что заставляет сбрасывать все контексты и кэши по файлу.

Nikita Gryzlov, [05.11.2021 11:59]
если получится заставить не ребилдить всё и вся на второй раз, то можно сэкономить немало времени. да, дерево разбора и токенизацию делать все равно нужно, но вот тяжелые операции над индексом можно исключить.

Nikita Gryzlov, [05.11.2021 12:00]
типа если учитывать какой-то доп.флаг при анализе. или, например, передавать версию не 0, а что-нибудь типа -1, и в rebuild сделать отдельную его обработку

Nikita Gryzlov, [05.11.2021 12:01]
ну и учесть слой с эвентами - вызов перестройки референс индекса идет через аспект, а не прямым вызовом. но и это можно обработать.

@qtLex
Copy link
Contributor Author

qtLex commented Nov 8, 2021

Nikita Gryzlov, [05.11.2021 11:58] @qtLex кстати, есть вариант, как "обмануть" бэнч и в целом оптимизировать инициализацию контекста. сейчас референс индекс и символьное дерево при анализе по сути инициализируются по два раза на каждый файл - первоначально при populateContext они заполняются для всех файлов, а потом при анализе для каждого файла вызывается rebuild с version = 0, что заставляет сбрасывать все контексты и кэши по файлу.

Nikita Gryzlov, [05.11.2021 11:59] если получится заставить не ребилдить всё и вся на второй раз, то можно сэкономить немало времени. да, дерево разбора и токенизацию делать все равно нужно, но вот тяжелые операции над индексом можно исключить.

Nikita Gryzlov, [05.11.2021 12:00] типа если учитывать какой-то доп.флаг при анализе. или, например, передавать версию не 0, а что-нибудь типа -1, и в rebuild сделать отдельную его обработку

Nikita Gryzlov, [05.11.2021 12:01] ну и учесть слой с эвентами - вызов перестройки референс индекса идет через аспект, а не прямым вызовом. но и это можно обработать.

Посмотрел на описанную конструкцию. Понял как-то так. populateContext используется, что бы потокобезопасно наполнить коллекцию контекстов документов с версией 0. А когда она наполнена начинает делать это снова уже с версией 1 и проверки. Это делаем так как не все заполнили в первый раз для проверки? Или какая-то другая причина? События по заполнению индекса вроде вызвались тоже.

Тут могу на долго засесть. Я бы предложил заняться этим в отдельной ветке.

@nixel2007
Copy link
Member

nixel2007 commented Nov 10, 2021 via email

@qtLex
Copy link
Contributor Author

qtLex commented Nov 18, 2021

Хм... Возможно ошибка не в этом узле, а где-то глубже, и её отловит tree Contains Error. А что за ctx у тебя на входе? Какой узел? ср, 10 нояб. 2021 г., 18:43 Eduard Ivanov @.***>:

Я переносил место поиска после скоупа в VariableSymbolReferenceIndexFinder. Сейчас с проверкой на Trees.nodeContainsErrors(ctx) нормально получается, но есть одна особенность если есть ошибка разбора.

Тестируемый код

Код содержит ошибку разбора.

Процедура Выполнить()

    А = 10;

КонецПроцедуры

MethodSymbolComputer

ctx на входе BSLParser.ProcedureContext

(startNode == null || startNode instanceof ErrorNode || stopNode == null || stopNode instanceof ErrorNode) = false
Trees.nodeContainsErrors(ctx) = false
Trees.treeContainsErrors(ctx) = true

VariableSymbolReferenceIndexFinder

ctx на входе BSLParser.SubContext

Trees.nodeContainsErrors(ctx) = false
Trees.treeContainsErrors(ctx) = true

documentContext.getSymbolTree().getMethods() возвращает один метод символ. На нужный нам метод Выполнить.
А результат вызова documentContext.getSymbolTree().getMethodSymbol(ctx) равен Optional.empty.

SymbolTree

Сейчас добрался до того, что при поиске формируется subNameRange с какимито очень странными значениями

subNameNode = ((BSLParser.SubContext) ctx).procedure().procDeclaration().subName();
Range subNameRange = Ranges.create(subNameNode);

image

Проблема не в Trees.nodeContainsErrors(ctx) он сейчас ведет себя правильно. Судя по всему где-то внутри парсера как-то странно разбирается. Сейчас сильно не мешает, падение я убрал и сейчас в таких случаях скоуп становиться модуль.

@nixel2007
Copy link
Member

/rebase

@nixel2007 nixel2007 force-pushed the feature/in-memory-db-references-to-variables branch from 07da734 to 445342e Compare January 2, 2022 12:20
@nixel2007
Copy link
Member

stfu, benchmark

@nixel2007 nixel2007 merged commit c2b2ba5 into 1c-syntax:develop Jan 2, 2022
@nixel2007
Copy link
Member

Титанически. спасибо, @qtLex!

@qtLex qtLex deleted the feature/in-memory-db-references-to-variables branch January 4, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Неиспользуемая локальная переменная
2 participants