Skip to content

Split Node type into more appropriate types #33432

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

Closed
Swatinem opened this issue Sep 14, 2019 · 3 comments
Closed

Split Node type into more appropriate types #33432

Swatinem opened this issue Sep 14, 2019 · 3 comments
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status.

Comments

@Swatinem
Copy link
Contributor

I recently did some heap profiling and noticed that v8 makes all the Node objects the same size.
Even though TS internally differentiates between Node / Identifier / Token, v8 still treats them the same because they come from the same constructor function.

Splitting those up can lead give up to ~8% reduction in memory usage, as shown in my PR here: #33431

I also noticed that the Token type is not really a good fit, since also literals like numbers, strings, boolean, etc are of type Token, but they do participate in type-checking and thus get additional properties later on, which is not ideal for performance (although my PR did not show any significant change).

So my questions here are:

  • Should we micro-optimize this? I would argue yes, since my very simple change already shows great memory usage savings.
  • What is a more appropriate grouping of Node types? I would argue putting syntax-only nodes, such as true Tokens (punctuation, operators, etc) and keywords into their own category, since they have no influence on checker performance.
  • Depending on properly split Node types, what properties should each type have? For example, syntax-only Nodes do not need an id, but they still have now because of literals.

So far, I only looked at typecker performance when run through tsc. So far, I have no idea how tsserver works with regards to incremental parsing and checking. Is the type of a node immutable on incremental parsing / checking?

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Sep 16, 2019
@RyanCavanaugh
Copy link
Member

@rbuckton this looks like a promising angle of analysis - please follow up on the associated PR

@Swatinem
Copy link
Contributor Author

Swatinem commented Sep 17, 2019

BTW, here is a more detailed list of all the Nodes (SyntaxKind) that get created when compiling the ts compiler:

Oh github, Y U NO fenced code blocks inside of <details>?¿?

  1:     141
  8:  30,201
 10:  11,383
 13:      65
 14:      25
 15:     151
 16:      88
 17:     151
 24:  70,971
 25:     425
 27:      60
 28:     996
 30:     268
 31:     126
 32:     230
 35:   4,280
 36:   1,358
 37:   1,148
 38:     904
 39:     324
 40:      19
 41:       4
 42:       8
 43:       5
 46:     453
 47:      16
 48:       4
 49:   2,089
 50:   1,118
 51:       6
 52:      11
 54:   4,046
 55:   3,421
 56:   8,234
 57:   2,428
 60:  10,895
 61:     245
 62:       2
 63:       1
 67:       1
 70:      53
 71:     342
 72:       7
 73: 323,907
 78:     344
 86:   7,210
 88:   1,402
 94:       4
 95:       4
 97:     742
101:     138
103:   1,588
107:   3,293
114:      27
115:       4
116:       2
117:      43
121:   1,075
124:   3,429
126:     526
133:      73
134:     616
136:   5,177
137:      49
139:   6,392
140:     169
142:   3,878
144:      34
147:     105
149:   1,988
150:      87
151:   4,514
152:  49,256
154:  10,118
155:     295
156:   4,132
157:   1,990
158:      88
159:      11
160:       2
161:      44
162:     100
163:      64
164:     756
165:  42,527
166:   3,903
167:      72
168:     121
169:     641
170:   3,977
171:     157
173:       2
174:   6,856
175:     171
176:      41
177:      13
178:     262
179:   1,311
180:   1,780
181:     166
182:      26
183:   2,784
184:       3
185:     249
186:      34
187:     590
188:   1,190
189:   1,538
190:  60,681
191:   5,095
192:  37,215
193:      86
194:       2
195:   3,034
196:   4,413
197:   1,553
198:   4,423
199:       6
200:      91
201:     242
203:   4,270
204:     959
205:  33,534
206:   2,861
207:     159
208:       9
209:      91
210:       3
211:       2
212:   1,142
213:   1,011
214:     975
216:      30
217:     248
219:  28,823
220:  22,830
221:       1
222:  17,531
223:  13,239
224:      13
225:     368
226:     998
227:      60
228:     623
229:     193
230:     622
231:  13,644
233:     726
234:      22
235:      19
236:      49
237:       1
238:  16,191
239:  24,181
240:  24,794
241:     114
242:   2,927
243:     716
244:     361
245:     414
246:     398
247:     723
250:      51
251:      51
252:      34
253:      17
254:      28
255:       6
256:      29
257:      29
258:       2
272:   5,727
273:     229
274:     956
275:      33
276:   5,801
277:     703
278:      21
279:   5,265
280:       1
282:       1
285:     350
286:      26
287:       1
288:       2
289:       4
292:       1
293:       1
298:   4,309
301:     253
307:   2,540
308:      69
315:     877
316:   6,924
317:       1
318:      15
319:     199

@Swatinem
Copy link
Contributor Author

I think this was tried and didn’t lead to any more wins. In any case, I won’t be working on this anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

No branches or pull requests

3 participants