-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Reduce much memory usage #405
base: develop
Are you sure you want to change the base?
Conversation
You can also use LocalDate and LocalDateTime , which are faster and use less memory. |
I love ThreeTenBP and I've already utilized it in my app, since it is an excellent backport for Java 8 time API. However, it is based on immutable classes as Java 8 time API documents. So it's not appropriate to be used in onDraw method that would be called many times per second, as it would return new instances everytime it's modified, which put much pressure on GC. |
I don't know about onDraw, but it uses much less memory and is faster in terms of performance, at least in my tests... You can combine your way, with usage of LocalDate and LocaleDateTime. See here: Code:
|
Ok, I will do a more detailed benchmark on this. The point here actually isn't lying in Calendar or ThreeTenBP, it's the the newing instance behavior in onDraw method. Many instances are created, so GC will be busy clearing these instances and lag happens. What I did in this PR is caching the time instance in term of long primitive value which is much cheaper than objects (yes, LocalDate can do this, but not necessary though, and this lib uses Calendar everywhere, moving this lib from Calendar to threetenbp is another topic). And I observed a relief for frequent GC in my fork version and less laggy in my app. |
You can also check the memory usage. Of course, the above test is for LocalDate. In some cases, you would want to use LocalDateTime instead. Depends on the case. Also, sadly, the library requires initialization , but as I've found out, you can avoid it in many cases, as long as you don't need times zones (the initialization is of time zones loading). In the current library of viewing a calendar, it's not needed to load zones, so I'm very sure you can avoid the initialization. |
Timezone supporting is in another PR though. see #272 |
Do you think there should be a time zone here? |
Ah, I mean timezone things may be discussed in that PR. This PR just focuses on performance improvement and reducing API change as much as possible. And thr purposal for using ThreeTenBP doesn't seem to help improving performance bottleneck much in this PR (see my comment edited above), maybe you can open another PR for using ThreeTenBP? |
To me I don't see performance issues yet, and my main tasks is to make it as similar as possible to what the designer told, together with snapping by week. I've made a pull request here, including some improvements and even conversion to Kotlin : But it seems this library is actually not maintained anymore. Do you know perhaps how to set it to scroll horizontally with snapping of a whole week? I've also tried to go over all of the forks, but none had it. It seems this fork is most maintained currently, but it doesn't have it, and it actually has scrolling issue (reported here), yet is fixes another (here) |
I got ur idea then, u mean scrolling from week to week? This is a feature in my app, I use a work around:
|
@CTKnight See my answer here: |
Say, can you please set the new changes on my fork? It just got quite different from original repository, as I've converted it to Kotlin and all... |
Weird thing is that even when I clone your repository, it still is quite slow on old devices (Nexus 4 with Android 4.4.4) which run Google Calendar app just fine. |
@AndroidDeveloperLB I'm afraid I'm not available for this until September... As to the performance, this PR only remedies the allocation of Calendar class, but the StaticLayout class as mentioned is not included, so there is still allocation in onDraw method though it's much less than Calendar class. |
Well it's a start... |
I have a PR using Android 26, removed all java.util.Calendar usage and replaced with java.time. Please take a look Some work still needs to be done, but for my purposes it's enough. |
Oh too bad. You forked from Java instead of what I did here... Then again, I don't think I told you about it. Also, it's not recommended to use the built in class of LocalDateTime, because it requires a very new Android API version. You could have used one of the libraries I've mentioned here . How can I try what you've made? clone this : https://github.com/jlurena/Android-Week-View ? |
@AndroidDeveloperLB yep, and no I wasn't aware. I just saw this issue earlier today :\ And yes I understand that. The app I'm working on however will only work with devices with Android >= 26 |
I have made tons of changes, fixes, and improvements. |
Android-Week-View is a great library that I really love.
However, I come across a few problem when using this library:
Calendar.getInstance()
duringonDraw()
methodnew StaticLayout()
duringonDraw()
methodnew float[]
duringonDraw()
methodthese codes are not recommended by Android View development document because
onDraw
is called VERY frequently , so even if allocation of a very small size will be drastically amplified, which leads to frequent GC and GC will stop the drawing process and make the view laggy.here is my PR:
Calendar.getInstance()
by using someprivate static final Calendar
, so no more extra allocation duringonDraw()
EventRec
is not changed, the old StaticLayout will be retained and used.I have try my best to not to modify the API, It just take 4 lines change in the demo.