Christopher Blizzard (:blizzard) Assignee
	Description • 22 years ago

Should combine these into a single XInternAtoms() call if we can.  Keithp knows
the names of the atoms in question.

	keithp
	Comment 1 • 22 years ago

Here's a list of all of the atoms that Mozilla uses at startup time.  If
XInternAtoms was called before they were used, a single round trip would collect
all of the names and stick them into the Xlib cache.  Subsequent XInternAtom
calls from Mozilla or Gtk+ would then not require a separate round trip.

This may seem like a petty performance improvement, but with Blizzard's current
Gtk2 codebase, this turns out to be one of the most time consuming part of
application startup.

MANAGER
UTF8_STRING
WM_CLIENT_LEADER
WM_DELETE_WINDOW
WM_LOCALE_NAME
WM_PROTOCOLS
WM_TAKE_FOCUS
XdndAware
_MOTIF_DRAG_RECEIVER_INFO
_MOZILLA_COMMAND
_MOZILLA_LOCK
_MOZILLA_RESPONSE
_MOZILLA_USER
_MOZILLA_VERSION
_NET_WM_DESKTOP
_NET_WM_ICON
_NET_WM_ICON_NAME
_NET_WM_NAME
_NET_WM_PID
_NET_WM_PING
_NET_WM_STATE
_NET_WM_STATE_FULLSCREEN
_NET_WM_STATE_MAXIMIZED_HORZ
_NET_WM_STATE_MAXIMIZED_VERT
_NET_WM_STATE_STICKY
_NET_WM_WINDOW_TYPE
_NET_WM_WINDOW_TYPE_NORMAL
_XSETTINGS_S0
_XSETTINGS_SETTINGS

	Brian Tarricone
	Comment 2 • 21 years ago

i'm not sure if this is the proper location in the code to do this, but i
figured the call should be made right after gtk_init() (and thus the connection
to the X server is opened).  it _seems_ to make startup a little faster (on my
gtk2 build), but that is more than likely just wishful thinking/my imagination.

	Olivier Cahagne
	Comment 3 • 20 years ago

can we get this to bake on a Linux or Sun tinderbox to verify the performance
improvement ?

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 4 • 20 years ago

can you replace the hardcoded 29 with NS_ARRAY_LENGTH(cache_atoms)? that makes
it more obvious where the number comes from, and avoids having to change it when
new items are added.

	Vidar Haarr (not reading bugmail)
	Comment 5 • 19 years ago

biesi: Can you review this?
Who could sr? blizzard says "not doing reviews".

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 6 • 19 years ago

Comment on attachment 196835 [details] [diff] [review]
version 0.2

sorry, I'd rather not. please try someone who actually knows X11.

	Vidar Haarr (not reading bugmail)
	Comment 7 • 19 years ago

Comment on attachment 196835 [details] [diff] [review]
version 0.2

Maybe bryner? I have no idea, really.

	Brian Ryner (not reading)
	Comment 8 • 19 years ago

Comment on attachment 196835 [details] [diff] [review]
version 0.2

How much of a perf win is this?

	Vidar Haarr (not reading bugmail)
	Comment 9 • 19 years ago

Comment on attachment 196835 [details] [diff] [review]
version 0.2

I have an Seamonkey objdir build with the patch applied, and run
measure-simple.pl. Revoke the patch and go into 'obj/xpfe/bootstrap', execute
'make' and run measure-simple.pl again.

If that is the right way to do it, the startup win here is almost nonexistent
(around 0.01sec average from 3 startups).

	keithp
	Comment 10 • 19 years ago

The performance win in the local case will be negligible, but in the remote case
you've eliminated 28 round trips. Multiply that by the RTT for your connection
and you get a good idea of the startup performance improvement.

	Vidar Haarr (not reading bugmail)
	Comment 11 • 19 years ago

Comment on attachment 196835 [details] [diff] [review]
version 0.2

Should we do this on Mac, too?

Also, maybe cache_atoms should be const.

	Brian Ryner (not reading)
	Comment 12 • 19 years ago

Comment on attachment 196835 [details] [diff] [review]
version 0.2

Code-wise it's fine, but I think it's hard to read if you're looking through the startup sequence and then run into this giant list of atoms.  I think what I'd prefer is if the atom list was defined in a separate header file (exported from widget/src/gtk2), which nsAppRunner pulls in.  The array should be |static const char * const cache_atoms[] = { }|, too (you don't need to explicitly put the length when intializing an array this way).

	Sylvestre Ledru [:Sylvestre]
	Comment 13 • 7 years ago

We removed gtk2 in bug 1278282. Even if this bug might still be relevant with gtk3, it has been 10 years...