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

Unit.swift Store units #2371

Closed
wants to merge 9 commits into from
Closed

Unit.swift Store units #2371

wants to merge 9 commits into from

Conversation

e78l
Copy link
Contributor

@e78l e78l commented Jun 21, 2019

Reduced code size some w/single initializer in Dimension extension (vs 1 init/Unit* class). Added to size with big enum (stats off Ubuntu below) and other minor changes.

This would be a first step in the Measurement+DateComponents formatter implementation I would like to propose: e78l/swift-corelibs-foundation@613b64b

After PR:
text data bss dec hex filename
8773109 386873 151560 9311542 8e1536 libFoundation.so

Before PR, off 17912de
text data bss dec hex filename
8711549 386873 151528 9249950 8d249e libFoundation.so

Store units so each is only created once
Remove redundant initializers

// Store is private, access members by `stored(at:)`
// Set members by `stored(_,at:)`
private static var ObjectStore = [Unit?](repeating: nil, count: StoreIndex.max)
Copy link
Contributor

Choose a reason for hiding this comment

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

Style: lowercase. objectStore. Maybe cachedDimensions, storedDimensions?

private static var ObjectStore = [Unit?](repeating: nil, count: StoreIndex.max)

fileprivate static func stored(at index: StoreIndex) -> Unit? {
return ObjectStore[index.rawValue]
Copy link
Contributor

Choose a reason for hiding this comment

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

These aren't thread-safe, and likely need a lock.

Copy link
Contributor

@millenomi millenomi left a comment

Choose a reason for hiding this comment

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

@itaiferber what do you think?

@millenomi
Copy link
Contributor

@swift-ci please test

@millenomi
Copy link
Contributor

@e78l Are you still interested in this patch?

@millenomi
Copy link
Contributor

@swift-ci please test Linux

1 similar comment
@millenomi
Copy link
Contributor

@swift-ci please test Linux

@e78l
Copy link
Contributor Author

e78l commented Jul 22, 2019

Hoping to push the changes this week

@e78l e78l force-pushed the measfmt-pt2 branch 3 times, most recently from 1451951 to 2262826 Compare July 22, 2019 22:27
@shahmishal shahmishal closed this Oct 6, 2020
@shahmishal
Copy link
Member

The Swift project moved the default branch to main and deleted master branch, so GitHub automatically closed the PR. Please re-create pull request with main branch.

More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412

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.

3 participants