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

Optimize semantic token extraction logic #4050

Merged

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented Feb 4, 2024

The following idea comes from wz1000.
A follow up of #3958 , we have added a tokenizor to walk the hieAst along with the file rope, it means we no longer need to do the detour of storing temperal result as Map Range (Set identifier), instead we can optimize by fusing most of the logic into tokenizer and return [(Range, HsSemanticTokenType)] directly.

@soulomoon soulomoon self-assigned this Feb 4, 2024
@soulomoon soulomoon linked an issue Feb 4, 2024 that may be closed by this pull request
@soulomoon soulomoon marked this pull request as ready for review February 4, 2024 21:15
@soulomoon soulomoon requested a review from wz1000 February 4, 2024 21:16
rope :: !Rope -- the remains of rope we are working on
, cursor :: !Char.Position -- the cursor position of the current rope to the start of the original file in code point position
, columnsInUtf16 :: !UInt -- the column of the start of the current rope in utf16
, rangeHsSemanticList :: [(Range, HsSemanticTokenType)] -- (range, token type) in reverse order
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we move this out the state? It seems like all the other Tokenizer functions return (), instead we could just return [(Range, HsSemanticTokenType)]

Copy link
Collaborator Author

@soulomoon soulomoon Feb 5, 2024

Choose a reason for hiding this comment

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

Sure, maybe we can do a CPS to collect them one by one or switch to a structure with better concatenation time complexity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DList fit into it. I've switch to Dlist and remove rangeHsSemanticList out the state

@soulomoon soulomoon requested a review from wz1000 February 5, 2024 22:40
@soulomoon
Copy link
Collaborator Author

cc @wz1000
see if it is looking good now?

@wz1000
Copy link
Collaborator

wz1000 commented Feb 7, 2024

Yes. I'm ambivalent on the DList, but whatever seems cleaner to you. We can go back to the old approach with the state too if you prefer that.

@soulomoon
Copy link
Collaborator Author

Yes. I'm ambivalent on the DList

Just curious, I wonder why

@wz1000
Copy link
Collaborator

wz1000 commented Feb 7, 2024

Now that I see it, I wonder if maybe the previous approach was cleaner. But it is totally up to you!

@soulomoon
Copy link
Collaborator Author

soulomoon commented Feb 7, 2024

I see, I review it a bit, both should have simliar cleaness(DList cut a few lines), DList seems make the type look better. I'll stick with DList, seems more explicit in the type level.

@soulomoon soulomoon enabled auto-merge (squash) February 7, 2024 15:36
@soulomoon soulomoon merged commit 3c511b0 into master Feb 7, 2024
39 checks passed
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.

Optimization to semantic tokens extracting logic
2 participants