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

优化Properties#stringPropertyNames处理,jdk9以下版本此方法cpu消耗相对过高. #1072

Merged
merged 1 commit into from
May 3, 2018

Conversation

wuwen5
Copy link
Contributor

@wuwen5 wuwen5 commented Apr 26, 2018

@codecov-io
Copy link

codecov-io commented Apr 26, 2018

Codecov Report

Merging #1072 into master will increase coverage by 17.43%.
The diff coverage is 88.88%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #1072       +/-   ##
=============================================
+ Coverage     31.21%   48.65%   +17.43%     
- Complexity      111     1680     +1569     
=============================================
  Files            39      365      +326     
  Lines           977    10174     +9197     
  Branches        122     1030      +908     
=============================================
+ Hits            305     4950     +4645     
- Misses          628     4860     +4232     
- Partials         44      364      +320
Impacted Files Coverage Δ Complexity Δ
...trip/framework/apollo/internals/DefaultConfig.java 79.12% <88.88%> (ø) 26 <2> (?)
...ctrip/framework/apollo/portal/entity/bo/Email.java 0% <0%> (ø) 0% <0%> (?)
...ollo/portal/controller/ServerConfigController.java 8.33% <0%> (ø) 1% <0%> (?)
...ring/boot/ApolloApplicationContextInitializer.java 91.3% <0%> (ø) 5% <0%> (?)
...mework/apollo/biz/service/BizDBPropertySource.java 82.35% <0%> (ø) 12% <0%> (?)
...o/portal/spi/defaultimpl/DefaultLogoutHandler.java 16.66% <0%> (ø) 1% <0%> (?)
...amework/apollo/internals/ConfigServiceLocator.java 85.91% <0%> (ø) 12% <0%> (?)
...rip/framework/apollo/biz/service/AdminService.java 100% <0%> (ø) 2% <0%> (?)
...k/apollo/portal/controller/UserInfoController.java 8.33% <0%> (ø) 1% <0%> (?)
...ework/apollo/spring/util/BeanRegistrationUtil.java 72.72% <0%> (ø) 3% <0%> (?)
... and 317 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c494690...96efd8f. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 52.221% when pulling 962e4ef8c0576ba40f83a1b1feeea832299ac4d8 on wuwen5:properties#preform into dc65300 on ctripcorp:master.

@wuwen5 wuwen5 force-pushed the properties#preform branch from 3204d46 to 72067bf Compare April 27, 2018 06:09
@wuwen5 wuwen5 changed the title 优化Properties#stringPropertyNames处理,jdk9以下版本此方法cpu消耗相对过大. 优化Properties#stringPropertyNames处理,jdk9以下版本此方法cpu消耗相对过高. Apr 27, 2018
import java.util.Objects;
import java.util.Properties;
import java.util.Set;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

这里似乎是IDE自动更新为*了,建议只import实际需要的package


private Set<String> stringPropertyNames(Properties properties) {
//jdk9以下版本Properties#enumerateStringProperties方法存在性能问题,keys() + get(k) 重复迭代, jdk9之后改为entrySet遍历.
Map<String, String> h = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

这段代码虽然是从jdk9复制过来的,还是建议加一段UT来测试,以防后续被其它人改错逻辑。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok已增加.

@wuwen5 wuwen5 force-pushed the properties#preform branch 2 times, most recently from 0c5c69b to 4e25576 Compare May 1, 2018 13:14
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

感谢!

ut的assertEquals应该可以利用现有的stringPropertyNames实现来比较。

另外,是否能再增加一个properties为空的test case?


Set<String> propertyNames = defaultConfig.getPropertyNames();

assertEquals(10, propertyNames.size());
Copy link
Member

Choose a reason for hiding this comment

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

assertEquals(someProperties.stringPropertyNames(), propertyNames) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已增加.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@wuwen5 wuwen5 force-pushed the properties#preform branch from 4e25576 to 96efd8f Compare May 3, 2018 02:08
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

Thanks!

@nobodyiam nobodyiam merged commit f1f7245 into apolloconfig:master May 3, 2018
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.

4 participants