[SwiftSyntax] Refine several aspects of BumpPtrAllocator.

[pinkjuice66]

While skimming the BumpPtrAllocator class, I identified areas for improvement.

• Rename slapSize to initialSlapSize : The variable name slabSize is quite confusing because it accidently implies that the allocated amount at a time is fixed, which is not the case. BumpPtrAllocator calculates the next size to be allocated based on the current index of the slabs array. So, I think we should clarify that it's only used initially.
• Remove unnecessary cleanup : Removed some unnecessary cleanup in the deinitializer of BumpPtrAllocator.
• Clarify the conditions for allocating a custom slab : We should check it based on the slabSize, not the initialSlabSize.

[rintaro]

I'm not sure about this, the current semantics of "custom size slab" is for reasonably big allocations.

For example, say the init slab size is 1024, the current slab size is 4096, and we already used 128 bytes of the current slab. If the next allocation requested 4000 bytes, the current slab cannot hold it, the next can, but if we do that, we'd waste 3968 bytes of the current slab. Custom slabs are to avoid that kind of waste.

[pinkjuice66]

That makes sense.
But in the case of your example, if the allocator continues to be requested 4000 bytes, a memory allocation occurs as per the request, and I think this should be avoided. I'm not sure if that could be problematic in our use cases because I'm quite new to this project. If so, I suppose we need a more sophisticated mechanism to ensure that we minimize memory waste and avoid frequent memory allocations.
If that's not a problem in our use cases, do you think it would be better to clarify that we do our best not to waste memory here? If so, I'll add some comments.
@rintaro

[rintaro]

Each object allocation in SwiftSyntax is below 64 bytes. Layout/collection data can be long but it's just (pointer size * number-of-elements), so several hundreds won't be an issue in ParsingArena (slabSize == 4096). Another one is the source buffer which totally makes sense to use a custom size slab. For non-parsing arena (sizeSize == 128) which usually used for editing, memory allocation is not a dominant factor in performance, afaik.
We also use BumpPtrAllocator in ASTGen, but it's similar.

We could make an option to change the behavior, but I don't think it's worth it at this point.

(FWIW, the current behavior resembles the default behavior of llvm::BumpPtrAllocator's SizeThreshold which defaults to SlabSize )

[ahoppen]

Looks good to me but I agree with @rintaro’s comment.

[rintaro]

@swift-ci Please test

[rintaro]

/home/build-user/swift/lib/ASTGen/Sources/ASTGen/ASTGen.swift:79:66: error: incorrect argument label in call (have 'slabSize:', expected 'initialSlabSize:')
  fileprivate let allocator: SwiftSyntax.BumpPtrAllocator = .init(slabSize: 256)
                                                                 ^~~~~~~~~
                                                                  initialSlabSize

@pinkjuice66 Please update ASTGen too.

[pinkjuice66]

I suppose ASTGen.swift resides in the swift repository.
Should I open a pull request on the swift repository first and then merge this pull request on our side?
I believe their CI might also fail. What would you suggest I do in this situation?
@rintaro

[rintaro]

@swift-ci can do cross repo PR testing. Could you just open a PR for ASTGen changes in swift repo?

Here's the detail how we handle cross repo changes: https://github.com/apple/swift/blob/main/docs/ContinuousIntegration.md#cross-repository-testing

[rintaro]

swiftlang/swift#71210
@swift-ci Please test