Scanner(unification) + TryPeek #1463

AdamSpeight2008 · 2015-03-21T19:54:17Z

This pull request unifies the contents of TryScanToken and ScanTokenFullWidth.
I retain those methods so not the break existing calls to them. The internals now call a common implementation method ScanTokenCommon.

This reduces the size of the sourcecode and the resulting IL (Debug)

                       Old |  New 
                     ------+------
      TryScanToken:-  1746 |   42
ScanTokenFullWidth:-  1972 |   15
   ScanTokenCommon:-       | 2035
                     ------+------
             Total:-  3718 | 2092

gafter · 2015-03-27T21:55:43Z

@AlekseyTs Could you please look at this and decide if it is worth moving forward with it?

AdamSpeight2008 · 2015-03-31T04:01:32Z

@dotnet-bot test this please

AdamSpeight2008 · 2015-04-01T10:44:27Z

Resubmit within PR #1727 as that PR merge is simpler.

AlekseyTs · 2015-04-01T17:44:34Z

src/Compilers/VisualBasic/Portable/Scanner/CharacterInfo.vb

+    Friend Module CharExts
+
+        <Runtime.CompilerServices.Extension>
+        Friend Function IsAnyOf(c As Char, c0 As Char, c1 As Char) As Boolean


Consider changing the name to something more descriptive. For example, IsEqualToAny.

What about if I add XML documentation to them instead?

Unfortunately, XML documentation doesn't help at a call site unless you are reading the code in VS.

Considering that LNQ provides the extension method .Any, I don't the contextual meaning isn't that far of a leap.

I am simply suggesting to name the methods in such a way so that no leap would be necessary.

AdamSpeight2008 · 2015-04-01T20:46:58Z

@dotnet-bot test this please

AlekseyTs · 2015-04-01T21:01:06Z

src/Compilers/VisualBasic/Portable/Scanner/Scanner.vb

                        ' TODO: do we allow widechars in keywords?
                        Dim spelling = "As"
-                        AdvanceChar(2)
+                        If fullWidth Then spelling = GetText(2) Else AdvanceChar(2)


Consider using multi-line If

I've had the feeling that this could be simplified to

AdvanceChar(2) Return MakeKeyword(SyntaxKind.AsKeyword, "If", precedingTrivia)

or (currently testing)

Return MakeKeyword(SyntaxKind.AsKeyword, GetText(2), precedingTrivia)

but never entirely sure.

Private Function GetText(length As Integer) As String Debug.Assert(length > 0) Debug.Assert(CanGet(length - 1)) If length = 1 Then Return GetNextChar() End If Dim str = GetText(_lineBufferOffset, length) AdvanceChar(length) Return str End Function

@AlekseyTs I've tried both alternatives and both pass the unit test.
So I'm going with the first alternative.

AdvanceChar(2) Return MakeKeyword(SyntaxKind.AsKeyword, "If", precedingTrivia)

As it calls less code.

This will then apply to the other cases, as well with the appropriate changes.

AdamSpeight2008 · 2015-05-31T10:10:13Z

@jasonwilliams200OK Done and Thank you for the notes on the rebase.

ghost · 2015-05-31T11:38:23Z

👍
@AlekseyTs, all yours. IMO this PR (with highest number of comments) is ready to bring (probably little but) useful value to the roslyn code base. Please consider concluding it before it requires resolving merge conflicts again. :)

AdamSpeight2008 · 2015-05-31T11:42:31Z

It makes the vb parser / scanner code a lot nicer to work with. Some section are little more efficient, some occur minor costs for the sake of clarity of code. I've had to resolve merge conflict twice already.

AlekseyTs · 2015-06-01T16:47:45Z

@VSadov, @gafter, @agocke, @jaredpar, @khyperia FYI

AdamSpeight2008 · 2015-06-01T17:06:00Z

src/Compilers/VisualBasic/Portable/Scanner/Scanner.vb

-                ch = MakeHalfWidth(ch)
-                Return ScanTokenFullWidth(precedingTrivia, ch)
+            If fullWidth Then
+                Debug.Assert(Not IsDoubleQuote(ch))


Debating whether it is worth making this as debug only? It be an empty branch (true) body in release mode..

gafter · 2015-06-01T22:23:38Z

I think the concerns about overuse of AggressiveInlining should be addressed.

I agree with Jared that we should avoid contractions in method names.

I am concerned that this PR combines too many changes to be effectively reviewed.

jaredpar · 2015-06-01T22:25:49Z

I agree with @gafter

AdamSpeight2008 · 2015-06-02T00:28:29Z

@gafter Strange I thought I'd already addressed those, unless the squash zapped them.

…Exception. Use of string interpolation.

Rename method so it does't use contraction. Remove Inlining Attribute. Restructured and IF statement in ScanTokenCommon

AdamSpeight2008 · 2015-06-02T21:32:14Z

@dotnet-bot test this please

AdamSpeight2008 · 2015-06-02T22:53:26Z

@gafter @jaredpar @AlekseyTs @jasonwilliams200OK @ljw1004

Hasn't this been slowly reviewed? over the course of its inception to now, through it varied incarnations. A little tweak there another tweak here, since it involve a delicate part of the VB.net compiler. Even if if doesn't make it into current release, it is worth looking at a future one. I think that progression has had side benefits, of us discovering other issues, gotchas and problems in the language specification.

gafter · 2015-06-02T23:02:27Z

@AdamSpeight2008 Because this pull request is so large, and has been presented in numerous revisions, I have no idea which parts have been reviewed and which not, and which parts have outstanding issues. It is too large for us to reasonably scan through it to answer such questions. I'd prefer to see such changes presented in a series of separate PRs each focused on a particular set of issues.

gafter · 2015-06-02T23:04:46Z

@AdamSpeight2008 I do not have any idea why @jasonwilliams200OK was giving you instructions of what to do with this PR.

AdamSpeight2008 · 2015-06-02T23:09:06Z

@gafter They were helping me reduce the number of commits ( I think it was in the region of 20) in PR, since I seem to break thing when I do git commands.

AdamSpeight2008 · 2015-06-02T23:35:23Z

Quick overview of concern and issues

Formatting Changes
None / minor / end of line whitespace removed.
Use of Inlining attributes
Removed and not used.
Unified methods ScanTokenFullWidth and TryScanToken
implemented, Tested, Working. Size reduction of the sourcecode and IL code outputted
Implement and use TryPeek
Completed, iteratively tested (Pass)
Issue around usage of contractions in method names.
Methods now use the non-contracted forms.
Test Failures do to Locale Single Test Failure possibly due Locale. #1006
Doensn't occur now (see Localization fix for DiagnosticLocalizationTests.TestDiagnosticLocalizat... #1889)
Accept PR or parts of it (mentions)
@AlekseyTs 👍
@pharring
@theory wrote (source: (Proposal) Shorten Some Commonly Used Method Names. #999)

Nice To Have

@gafter wrote:

I can imagine accepting a pull request that does this, but it is hard to imagine the Roslyn team doing this themselves.

I propose that we accept this PR (with the possible addition of an attribute to encourage jit inlining) and keep an eye on the performance for any regression.

JIT Inlining
Performance

gafter · 2015-06-17T01:13:17Z

@AlekseyTs Can you please make an executive decision as to whether we want to move forward with this PR, or instead close it and request that the submitter re-submit smaller more focused PRs for the elements of it?

AdamSpeight2008 · 2015-06-17T10:57:06Z

Again?! True it's a big PR.

The Scanner(Unification) change affect 2/3 methods, but as the TryParse changes developed some of those changes got incorporated into the Scanner(Unification) methods. Submitting (yet another set of) PRs I don't think would be "better". If the number of comments on this PR (and previous) are anything to go by, have helped refine / improve the changes. It has stabilised and there hasn't been a changes in a while, if there anything that may need altering or change could be fixed in smaller subsequent Pull Requests.

gafter · 2015-06-17T14:02:18Z

@AdamSpeight2008 Not again. Still. One reason for the decreasing number of review comments and changes is that some reviewers have grown tired of re-reviewing thousands of lines of code multiple times as you revise the PR. This code has not been reviewed since any of the last 10 revisions (perhaps more), and I am not motivated to review it again because it is so large and keeps changing out from under me.

In any case, my question was for @AlekseyTs.

AdamSpeight2008 · 2015-06-17T14:53:15Z

@gafter Oh don't treat me like a doormat, to be walked all over.

It's not like the master branch has not changed under me as well.

I've had all of the variable names change. (after the PR was submitted )
Which invalidated PR merge would now be a build failure, prior it wasn't.
Rebase to current state of master
Squash the commits
Both of which came for "Team" members. "Team" member wanted changes / improvements which I've complied with. Hence additional commit to this PR
Been instructed to not to submit another PR that is essence this / these.
Recent commits are only to rebase to 'master`

See it from me side.
If I do the follow imaginaryl PRs

  1. Scanner Methods (Unified)
  2. TryParse Methods

Merge conflict as the 2nd contains the older methods. Thus I have rebase if
or

  1. TryParse Method
  2. Scanner Methods (Unified)

Merge conflict as the unified method doesn't contain the use of TryParse. Thus require addition commit / rebase.

Either way I'm screwed i terms merge conflicts

Alternative this

I submit yet another PR, lets say Scanner Methods (Unified).
Wait for the Yah / Nah.
Wait for the merge into master.
Fork with this new master as a base,
Redo all the TryParse implememtations. ( possible missing some / making errors )
Submit yet another PR.
Wait for the Yah / Nah.

Wouldn't it be simpler for everyone to have one PR (this one)?

jaredpar · 2015-06-18T16:47:31Z

@AdamSpeight2008

The issue with this PR is that it's a collection of changes rather than a single change to the scanner:

Formatting changes: white space removal, line break changes, etc ...
Refactoring: new helper methods: Peep, IsnotFrom, etc ... , rewriting core methods like ScanTokenFullWidth
Code hygene changes: switching to interpolated strings

Combining all of these changes into a single PR, and a large one at that, makes it significantly costlier for the Roslyn team to review and verify. The ask from @gafter, which I completely agree with, is for the PR to be broken into smaller focused PRs.

In its current form I don't see any way we could merge this PR into the source tree:

The scanner is a very perf sensitive area for the compiler and we don't treat refactoring changes to it lightly. Verifying the change involves careful monitoring of our perf suites plus an investigation to any deviations we see. This is why we are pushing to separate out the refactorings from all of the other changes such as formating, name changes, etc ... It simplifies the investigation process to have the change as minimal as possible.
I know you disagree with our formatting and style decisions but they are not going to change and we will not be accepting PRs that contain arbitrary reformattings of our code. I've mentioned this several times in the past.
The size of the change and mixed refactorings make it harder to follow. It's difficult to tell if parts of the change are bug fixes or changes that respond to refactorings you made.

There is a general question as to whether we would take the refactoring of the Scanner even if it was separated out from the rest of the change. Some of the pattern changes, like Peep, look good at least to me. Other changes are of questionable value and have a high cost for the team in terms of verifying the changes. Breaking the refactoring into smaller, easier to review, changes would significantly increase the likely hood we would accept it into our source tree.

I do want to thank you for pushing on this change. We are interested in adding value to our code bases and cleaning up parts of our code base which have gone stale. I appreciate that you've put a good deal of effort into this PR and are trying to add value to our code base. But in its current form it's not possible for us to accept it for the reasons listed above. Given all of this I'm closing this PR.

dnfclas added the cla-not-required label Mar 21, 2015

AdamSpeight2008 closed this Mar 21, 2015

AdamSpeight2008 mentioned this pull request Mar 26, 2015

Unification of the contents of TryScanToken and ScanTokenFullWidth. #1381

Closed

AdamSpeight2008 reopened this Mar 26, 2015

dnfclas added the cla-not-required label Mar 26, 2015

AdamSpeight2008 mentioned this pull request Mar 27, 2015

Rename commonly used method to a shorter version. #1627

Closed

gafter assigned AlekseyTs Mar 27, 2015

gafter added the Area-Compilers label Mar 27, 2015

gafter added this to the 1.0 (stable) milestone Mar 27, 2015

AdamSpeight2008 mentioned this pull request Apr 1, 2015

Scanner(Unification) & TryPeek #1727

Closed

AdamSpeight2008 closed this Apr 1, 2015

AdamSpeight2008 reopened this Apr 1, 2015

dnfclas added the cla-not-required label Apr 1, 2015

AdamSpeight2008 closed this Apr 1, 2015

AdamSpeight2008 reopened this Apr 1, 2015

dnfclas added the cla-not-required label Apr 1, 2015

AdamSpeight2008 force-pushed the Scanner(Unification) branch from e2db2e2 to 23a5f87 Compare April 1, 2015 16:55

AlekseyTs reviewed Apr 1, 2015
View reviewed changes

AdamSpeight2008 force-pushed the Scanner(Unification) branch from 343a889 to cc616ed Compare May 31, 2015 10:07

AdamSpeight2008 reviewed Jun 1, 2015
View reviewed changes

AdamSpeight2008 added 2 commits June 2, 2015 02:07

[VB: Compiler SymbolDisplay] Add the parameter name to a ArgumentNull…

a58d3f7

…Exception. Use of string interpolation.

[VB Compiler Scanner]

711246e

Rename method so it does't use contraction. Remove Inlining Attribute. Restructured and IF statement in ScanTokenCommon

AdamSpeight2008 force-pushed the Scanner(Unification) branch from 179bf0d to 711246e Compare June 2, 2015 01:14

AdamSpeight2008 added 2 commits June 2, 2015 02:19

[VB Compiler Scanner] Remove Inlining Attribute from some methods

1bb3643

Merge branch 'dotnet/master' into Scanner(Unification)

886f0f6

gafter added the 4 - In Review A fix for the issue is submitted for review. label Jun 16, 2015

jaredpar closed this Jun 18, 2015

AdamSpeight2008 deleted the Scanner(Unification) branch October 31, 2015 22:29

gafter added Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on. and removed 4 - In Review A fix for the issue is submitted for review. labels Dec 9, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scanner(unification) + TryPeek #1463

Scanner(unification) + TryPeek #1463

AdamSpeight2008 commented Mar 21, 2015

gafter commented Mar 27, 2015

AdamSpeight2008 commented Mar 31, 2015

AdamSpeight2008 commented Apr 1, 2015

AlekseyTs Apr 1, 2015

AdamSpeight2008 Apr 1, 2015

AlekseyTs Apr 1, 2015

AdamSpeight2008 Apr 1, 2015

AlekseyTs Apr 1, 2015

AdamSpeight2008 commented Apr 1, 2015

AlekseyTs Apr 1, 2015

AdamSpeight2008 Apr 1, 2015

AdamSpeight2008 Apr 1, 2015

AdamSpeight2008 commented May 31, 2015

ghost commented May 31, 2015

AdamSpeight2008 commented May 31, 2015

AlekseyTs commented Jun 1, 2015

AdamSpeight2008 Jun 1, 2015

gafter commented Jun 1, 2015

jaredpar commented Jun 1, 2015

AdamSpeight2008 commented Jun 2, 2015

AdamSpeight2008 commented Jun 2, 2015

AdamSpeight2008 commented Jun 2, 2015

gafter commented Jun 2, 2015

gafter commented Jun 2, 2015

AdamSpeight2008 commented Jun 2, 2015

AdamSpeight2008 commented Jun 2, 2015

gafter commented Jun 17, 2015

AdamSpeight2008 commented Jun 17, 2015

gafter commented Jun 17, 2015

AdamSpeight2008 commented Jun 17, 2015

jaredpar commented Jun 18, 2015

Scanner(unification) + TryPeek #1463

Scanner(unification) + TryPeek #1463

Conversation

AdamSpeight2008 commented Mar 21, 2015

gafter commented Mar 27, 2015

AdamSpeight2008 commented Mar 31, 2015

AdamSpeight2008 commented Apr 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamSpeight2008 commented Apr 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamSpeight2008 commented May 31, 2015

ghost commented May 31, 2015

AdamSpeight2008 commented May 31, 2015

AlekseyTs commented Jun 1, 2015

Choose a reason for hiding this comment

gafter commented Jun 1, 2015

jaredpar commented Jun 1, 2015

AdamSpeight2008 commented Jun 2, 2015

AdamSpeight2008 commented Jun 2, 2015

AdamSpeight2008 commented Jun 2, 2015

gafter commented Jun 2, 2015

gafter commented Jun 2, 2015

AdamSpeight2008 commented Jun 2, 2015

AdamSpeight2008 commented Jun 2, 2015

gafter commented Jun 17, 2015

AdamSpeight2008 commented Jun 17, 2015

gafter commented Jun 17, 2015

AdamSpeight2008 commented Jun 17, 2015

jaredpar commented Jun 18, 2015